Skip to content

Conversation

@Eldar-Ahmadov
Copy link
Contributor

…t complete the websocket reader pipeline

When a GraphQL client is subscribed and prematurely closes (e.g. the app crashes, or is simply closed), the WebSocketReaderPipeline tries to close the socket by calling _socket.CloseAsync which never returns.

Even though Microsoft's documentation on CloseAsync and CloseOutputAsync is quite similar, according to the below links:

CloseAsync waits for the client to acknowledge closing of the socket, whereas CloseOutputAsync works in fire-and-forget mode.

It looks like the use of those methods should be swapped to achieve the correct semantics, so that CloseAsync does not get stuck waiting for the client's acknowledgement (the client may already be closed by now) and hence never complete the reader pipeline.

Eldar Ahmadov added 2 commits November 8, 2018 17:14
…lock for cases when CloseAsync fails because socket is in Aborted state
@fayilt
Copy link

fayilt commented Nov 9, 2018

Well spotted!

@graphdb2
Copy link

Can someone merge this fix?

@pekkah
Copy link
Contributor

pekkah commented Nov 10, 2018

There's a formatting issue in the PR. Perhaps someone using tabs instead of spaces..

@Eldar-Ahmadov
Copy link
Contributor Author

Replaced tabs with spaces.

@pekkah
Copy link
Contributor

pekkah commented Nov 12, 2018

Still some formatting issues in the code. Functions wont align properly. Have you tried format document in VS?

@Eldar-Ahmadov
Copy link
Contributor Author

Hi Pekka. Yes, the code has been edited in VS. Fixed the alignment issue as well.
Thanks

@pekkah pekkah merged commit f7ae41f into graphql-dotnet:develop Nov 12, 2018
adinKent pushed a commit to Conning/graphql-dotnet-server that referenced this pull request Mar 4, 2021
graphql-dotnet#177)

* Fix the issue when the client prematurely closes and the server cannot complete the websocket reader pipeline
* Fix the issue when the client prematurely closed. Add a try/finally block for cases when CloseAsync fails because socket is in Aborted state
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.

4 participants