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.

Sign up for GitHub

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

Jump to bottom

No end stream on ws connect and flush every message #73762

Merged
merged 10 commits into from
Aug 12, 2022
Merged

No end stream on ws connect and flush every message #73762

merged 10 commits into from
Aug 12, 2022

Conversation

greenEkatherine
Copy link
Contributor

The second attempt to fix #72301

End stream should not be sent on the connect request, but it should be flushed instead.
Also, for web sockets we should flush every message immediately.

To test the change, I run all existing tests for 1.1 against local Kestrel server with WS over HTTP/2 support. All tests passed except CloseStatusDescription checks - it seems to be changed on the server side. Now server adds "Server received" and status at the beginning

@ghost ghost assigned greenEkatherine Aug 11, 2022
@ghost
Copy link

ghost commented Aug 11, 2022

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

Issue Details

The second attempt to fix #72301

End stream should not be sent on the connect request, but it should be flushed instead.
Also, for web sockets we should flush every message immediately.

To test the change, I run all existing tests for 1.1 against local Kestrel server with WS over HTTP/2 support. All tests passed except CloseStatusDescription checks - it seems to be changed on the server side. Now server adds "Server received" and status at the beginning

Author: greenEkatherine
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 11, 2022
@greenEkatherine greenEkatherine added this to the 7.0.0 milestone Aug 11, 2022
@CarnaViire
Apply suggestions from code review
fe2a960 
Co-authored-by: Natalia Kondratyeva <knatalia@microsoft.com>
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Aug 11, 2022
@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub
stephentoub reviewed Aug 11, 2022
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Not to sound like a broken record, but where are the tests?

@greenEkatherine
Copy link
Contributor Author

Not to sound like a broken record, but where are the tests?

Sorry, I'm again struggling to simulate "normal server" on the loopback server, so I ended up with manual testing on the local server. I tried to fix the test that should indicate closed connection though.

@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@greenEkatherine greenEkatherine mentioned this pull request Aug 12, 2022
Merged
@greenEkatherine
Copy link
Contributor Author

I opened an issue for tests #73849

CarnaViire
CarnaViire approved these changes Aug 12, 2022
View reviewed changes
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.

LGTM

Katya Sokolova and others added 2 commits August 12, 2022 16:47
@stephentoub
Apply suggestions from code review
b621b04 
Co-authored-by: Stephen Toub <stoub@microsoft.com>
feedback
12ff77f
@greenEkatherine
Copy link
Contributor Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@greenEkatherine
Copy link
Contributor Author

Failures in the outeloop are unrelated

@greenEkatherine greenEkatherine merged commit 30bec96 into dotnet:main Aug 12, 2022
@greenEkatherine greenEkatherine deleted the fix-end-stream-and-flush branch August 12, 2022 17:28
@ghost ghost locked as resolved and limited conversation to collaborators Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@CarnaViire CarnaViire CarnaViire approved these changes

@MihaZupan MihaZupan MihaZupan left review comments

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy

Assignees

@greenEkatherine greenEkatherine

Labels
area-System.Net.Http
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

WebSockets over HTTP/2 issues
4 participants
@greenEkatherine @stephentoub @CarnaViire @MihaZupan