Skip to content
New issue

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

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

Already on GitHub? # to your account

Handle multiple incoming connection at the beginning of Http3LoopbackConnection use #69453

Merged
merged 2 commits into from
May 24, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented May 17, 2022

Fixes #69387.

This PR makes sure the Http3LoopbackConnection used in tests does not choke on multiple simultaneous requests from a client, when two or more request streams are accepted before the control stream.

The Http functional tests were run in a tight loop and the original test failure was not observed.

@ghost
Copy link

ghost commented May 17, 2022

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

Issue Details

Fixes #69387.

This PR makes sure the Http3LoopbackConnection used in tests does not choke on multiple simultaneous requests from a client, when two or more request streams are accepted before the control stream.

The Http functional tests were run in a tight loop and the original test failure was not observed.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@rzikm rzikm requested a review from CarnaViire May 17, 2022 18:52
@rzikm
Copy link
Member Author

rzikm commented May 17, 2022

cc: @ManickaP

@ManickaP
Copy link
Member

Was the problem that not all unidirectional streams were the one and only control stream?

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

In general LGTM, but the only question is whether any synchronization needed over _delayedStreams access... can AcceptRequestStreamAsync ever be called concurrently? it feels like it might be...

@rzikm
Copy link
Member Author

rzikm commented May 24, 2022

Was the problem that not all unidirectional streams were the one and only control stream?

No, the problem was that client started multiple requests and it happened that two (or more) request streams arrived before the control stream.

@rzikm
Copy link
Member Author

rzikm commented May 24, 2022

In general LGTM, but the only question is whether any synchronization needed over _delayedStreams access... can AcceptRequestStreamAsync ever be called concurrently? it feels like it might be...

It does not seem that we are accepting multiple requests concurrently in any test. There are other parts of the simple loopback server that were not thread safe before (such as setting the _currentStream and _currentStreamId). Making it all threadsafe would require bigger changes and likely redesign of the class/tests.

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

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

Ok. If Http3LoopbackConnection is not thread-safe in general, I'm fine with this.
LGTM. Thanks!

@rzikm rzikm merged commit e86511e into dotnet:main May 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 23, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
4 participants