Skip to content

Let the user change the element-queue for AsyncQueue<T>#1606

Closed
TheConstructor wants to merge 1 commit into
microsoft:mainfrom
TheConstructor:feature/async-priority-queue
Closed

Let the user change the element-queue for AsyncQueue<T>#1606
TheConstructor wants to merge 1 commit into
microsoft:mainfrom
TheConstructor:feature/async-priority-queue

Conversation

@TheConstructor

Copy link
Copy Markdown

This allows the user to prioritize queued elements, or use an implementation otherwise different from Queue as backend.

If you think an abstract class is a better fit than an interface or I should change anything else, just reach out (or do it).

I would be happy, if I could use the nice async-wrapper that is AsyncQueue around a queue, that prioritises items and keeps insertion-order between items of the same priority (which the channel from Channel.CreateUnboundedPrioritized<T>() doesn't do). Now I don't think that this library needs to offer the whole thing, but AsyncQueue utilises some internal classes like TaskCompletionSourceWithoutInlining<T> that I would need to replace, if I want to have a full-fledged copy, without a change in this repository

This allows the user to prioritize queued elements, or use an implementation otherwise different from Queue<T> as backend.
Copilot AI review requested due to automatic review settings June 25, 2026 20:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends AsyncQueue<T> to allow callers to supply a custom synchronous queue implementation for element storage, enabling alternative ordering/priority behaviors while reusing the existing async coordination logic.

Changes:

  • Introduces a new public ISyncQueue<T> interface for synchronous queue operations.
  • Adds an internal SyncQueue<T> wrapper around Queue<T> as the default element store.
  • Adds an AsyncQueue<T> constructor that accepts a queue factory, plus a test validating a custom element queue can be used.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
test/Microsoft.VisualStudio.Threading.Tests/AsyncQueueTests.cs Adds coverage for using a custom element queue implementation; introduces a small test-only prioritized queue.
src/Microsoft.VisualStudio.Threading/SyncQueue`1.cs Adds the default internal Queue<T>-backed implementation used when no factory is supplied.
src/Microsoft.VisualStudio.Threading/ISyncQueue`1.cs Defines the new public abstraction that custom element queues must implement.
src/Microsoft.VisualStudio.Threading/AsyncQueue`1.cs Switches element storage to ISyncQueue<T> and adds a constructor for supplying a custom factory.

Comment on lines +35 to +38
/// <summary>
/// The factory for the internal queue of elements. A Queue`1 is constructed, when this is null.
/// </summary>
private readonly Func<int, ISyncQueue<T>>? elementsQueueFactory;
Comment on lines +70 to +74
/// <param name="elementsQueueFactory">Factory to create the queue used for items, if needed. Takes the initial capacity.</param>
public AsyncQueue(Func<int, ISyncQueue<T>> elementsQueueFactory)
{
this.elementsQueueFactory = elementsQueueFactory;
}
Comment on lines +19 to +23
/// <summary>
/// Adds an element to the tail of the queue.
/// </summary>
/// <param name="value">The value to add.</param>
void Enqueue(T value);
Comment on lines +31 to +35
/// <summary>
/// Gets and removes the element at the head of the queue.
/// </summary>
/// <returns>The head element.</returns>
T Dequeue();
Comment on lines +628 to +630
/// <summary>
/// This "prioritied" queue implementation is not optimized for speed, nor does it allow custom types or ordering.
/// </summary>
@AArnott

AArnott commented Jun 25, 2026

Copy link
Copy Markdown
Member

Thanks for your inclination toward sharing. The added abstraction around queue that you're adding appears to allow you to build a prioritized queue that you then plug into the system, but to this library, the abstraction adds no value without your plugin.
API has cost that must be offset by value, and the net impact of this PR in my estimation is negative.
IMO a prioritized queue is a significantly different use case than a simple queue and probably deserves a class of its own rather than as a plugin to an ordinary queue.
As such, I'm going to close the PR without merging.

As our OSS license is quite liberal, I suggest you copy the AsyncQueue code to make your own async prioritized queue for your needs.

@AArnott AArnott closed this Jun 25, 2026
@TheConstructor

Copy link
Copy Markdown
Author

As our OSS license is quite liberal, I suggest you copy the AsyncQueue code to make your own async prioritized queue for your needs.

Would it be an option to make TaskCompletionSourceWithoutInlining<T> public? I would really like to avoid unchanged copies, as I would prefer not watch out for bug-fixes that might take place at a later time.

If you would rather accept a PR containing an AsyncPriorityQueue I could do that as well, but I thought having the abstraction would be a better fit

@AArnott

AArnott commented Jun 26, 2026

Copy link
Copy Markdown
Member

TBH I don't know why we still have TaskCompletionSourceWithoutInlining<T>. I think we introduced it before .NET gave us RunContinuationsAsynchronously which this code is clearly using (probably modified to reuse rather than invent our own solution). Why not just use TCS<T> with the RunContinuationsAsynchronously flag?

@TheConstructor

Copy link
Copy Markdown
Author

I have to admit, that I have no clue, why TaskCompletionSourceWithoutInlining<T> copies base.Task in the constructor, but returns the copy only until the current base.Task is completed. Looked like a fix for one of those hard to pinpoint edge-cases. Looking at the source of TaskCompletionSource<T> that Task-property seems to always return the same instance in .NET 10 - so likely really not needed.

Which leaves ThreadingTools.AttachCancellation() as the last complicated internal reference, wich I would rather not want to maintain a copy of. And yes, that in turn references the private ThreadingTools.CancelableTaskCompletionSource, but that is a rather simple class. Would it be an option to make ThreadingTools.AttachCancellation() public?

@AArnott

AArnott commented Jul 9, 2026

Copy link
Copy Markdown
Member

@TheConstructor I filed #1617 to remove the obsolete class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants