Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QUIC] Does QUIC need sync versions of non-Stream APIs? #43792

Open
geoffkizer opened this issue Oct 24, 2020 · 11 comments
Open

[QUIC] Does QUIC need sync versions of non-Stream APIs? #43792

geoffkizer opened this issue Oct 24, 2020 · 11 comments
Labels
area-System.Net.Quic design-discussion Ongoing discussion about design without consensus
Milestone

Comments

@geoffkizer
Copy link
Contributor

We should have synchronous versions of APIs like AcceptConnection, AcceptStream, etc.

@scalablecory @stephentoub

@geoffkizer geoffkizer added this to the 6.0.0 milestone Oct 24, 2020
@ghost
Copy link

ghost commented Oct 24, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Oct 24, 2020
@scalablecory
Copy link
Contributor

I think we should optimize the sync methods on QuicStream, but I don't think we need sync methods anywhere else. What's the goal?

@geoffkizer
Copy link
Contributor Author

Maybe we don't. I'm not sure what our current general stance is on supporting sync APIs; I know there's some concern about not supporting them in general.

I'll defer to @stephentoub here.

@stephentoub
Copy link
Member

stephentoub commented Oct 24, 2020

At a minimum the sync stream methods should be implemented as sync all the way down.

It'd be good as well if the sync methods on HttpClient/SocketsHttpHandler were sync all the way down for HTTP/3 (rather than throwing as currently happens for HTTP/2), and I expect that would necessitate such sync connection methods.

Beyond that, I'd leave it up to expected use cases, e.g. do we expect existing sync code paths that can't be changed (e.g. some sync interface callback) would want to establish quic connections.

(All that said, I'm not currently clear on where blocking would occur with quick. If we'd still have sync over async due to multiplexing in the quic implementation, the benefits here may be limited, and we'll need to fully understand them to understand how much to invest here.)

@geoffkizer
Copy link
Contributor Author

If we'd still have sync over async due to multiplexing in the quic implementation, the benefits here may be limited, and we'll need to fully understand them to understand how much to invest here.

Yes, it would mean sync over async. Even for the sync stream methods.

I don't have a strong opinion here; I just want to make sure we are doing the right thing, and I'm not entirely clear on what that is.

It's also possible that the answer here may be different for the stream vs new QUIC-specific APIs.

@stephentoub
Copy link
Member

Yes, it would mean sync over async. Even for the sync stream methods.

At what level does the multiplexing happen? The worst case is only async methods exposed, and in a way where someone on a thread pool thread wants to do a sync operation and queues another work item that requires another thread pool thread to make forward progress, so you end up needing 2x the thread pool threads, waving my hands. At the other end of the spectrum, if all the multiplexing for all quic connections end up happening in one place, such that you only need at worst one additional thread (and potentially one that's not actually a thread pool thread, if it's actually a dedicated thread that's blocking synchronously), that's a good motivation for sync APIs. But I'm guessing reality is somewhere in the middle.

@geoffkizer
Copy link
Contributor Author

At what level does the multiplexing happen?

Per-connection, more or less.

So it's not a single global background task, but it's also not one per stream.

I'm guessing reality is somewhere in the middle.

Yep, exactly.

@scalablecory
Copy link
Contributor

MsQuic is purely async. It maintains its own thread (pool) in a mostly undocumented way. It fully abstracts multiplexing away from us.

I do think we can avoid the 2x explosion, at least, but we won't get much more efficient (I guess just avoiding the Task allocation) than just doing .AsTask().GetAwaiter().GetResult().

@scalablecory
Copy link
Contributor

Making H3 sync Send() work should be fairly trivial. The only part that might need refactoring is waiting for an available request stream (but, I think that specific piece of code needs some work anyway, so this doesn't bother me)

My opinion would be to optimize sync stream methods, but avoid introducing QUIC-specific sync APIs unless we believe there will be significant detriment to SocketsHttpHandler.Send. I don't think anyone should be using sync with QUIC.

@geoffkizer geoffkizer changed the title [QUIC] Add sync APIs [QUIC] Does QUIC need sync versions of non-Stream APIs? Nov 17, 2020
@karelz
Copy link
Member

karelz commented Jan 14, 2021

Triage: Given that we do not have any customers asking for it and it is non-trivial investment, we won't do it until we have strong business case in future.

@karelz karelz modified the milestones: 6.0.0, Future Jan 14, 2021
@karelz karelz added design-discussion Ongoing discussion about design without consensus and removed untriaged New issue has not been triaged by the area owner labels Jan 14, 2021
@karelz
Copy link
Member

karelz commented Oct 26, 2021

Triage: Still not needed from customers, pushing to Future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Quic design-discussion Ongoing discussion about design without consensus
Projects
None yet
Development

No branches or pull requests

5 participants