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

Streaming does not work in the new server arch #132

Closed
kazu-yamamoto opened this issue Jul 11, 2024 · 5 comments
Closed

Streaming does not work in the new server arch #132

kazu-yamamoto opened this issue Jul 11, 2024 · 5 comments

Comments

@kazu-yamamoto
Copy link
Owner

@edsko's comment copied from #130:

@kazu-yamamoto I just tried with 80de8db , same result as with aa6f979 . My test setup is that the client does a streaming request to the server; this request arrives, and the serves initiates a streaming response. The client receives the headers, but then nothing. If I insert a call to putStrLn in outBodyPush at

, outBodyPush = \b -> do
then I can see that print statement executed with the data that the client should receive, but the client does not receive anything after the response headers. There is a Wireshark log at well-typed/grapesy#183 where we're tracking updating to the new http2 architecture.

@edsko
Copy link
Collaborator

edsko commented Jul 11, 2024

Does the fact that you opened this issue mean that you can reproduce it also?

FWIW, @FinleyMcIlwaine did some digging, and wrote at well-typed/grapesy#183 (comment)

The server is hanging in waitForOutbound called from outboundTrailersMaker, which is called from http2 in fillDataHeaderEnqueueNext. The change in behavior seems to be that http2 is somehow constructing the trailers before grapesy is able to mark the outbound thread as done.

We'll continue to look into this (but I have another problem to sort out first).

@kazu-yamamoto
Copy link
Owner Author

No, I cannot reproduce it.
Thank you for the clue.

@edsko
Copy link
Collaborator

edsko commented Jul 11, 2024

@kazu-yamamoto we figured out what was happening. So http2 defines

type Server = Request -> Aux -> (Response -> [PushPromise] -> IO ()) -> IO ()

Let's call that third argument respond.

  • In the "old" architecture, respond returned when the response body was sent, but before the trailers were constructed or sent.
  • In the new architecture, this is not the case: respond only returns when the response body is sent and the trailers are constructed (and sent, but this is less relevant for us).
  • The TrailersMaker constructed by grapesy was waiting for respond to return (because it wanted to be sure the response body was complete), thus resulting in deadlock in the new architecture.

We changed grapesy in well-typed/grapesy#187 so that the TrailersMaker does wait for the last message to be sent but does not wait for the thread to finish, thus fixing the deadlock.

We have run the full grapesy test suite against the new http2 architecture and all our tests pass 🥳

@edsko
Copy link
Collaborator

edsko commented Jul 11, 2024

I think we can close this issue! :)

@kazu-yamamoto
Copy link
Owner Author

Thank you and sorry for your inconvenience.

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

No branches or pull requests

2 participants