search

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 112 forks source link

Handle settings after initial handshake #318

Closed lukebakken closed 1 year ago

lukebakken commented 3 years ago

Fixes #311

lukebakken commented 3 years ago

@ericmj @whatyouhide @voltone I spent a bit more time yesterday to understand what is happening in the code. It looks as though handle_settings should do the right thing if I remove this code from the initial negotiation.

However, it seems as though the test HTTP/2 server expects this negotiation to happen in a certain order, which of course this PR is changing. I'm assuming the test server needs to be tweaked a bit as well to send / handle the SETTINGS frame asynchronously.

If you have pointers on how you would proceed, that would be great. I'd like to get this done so that a 1.3 release could happen soon, because I have a finch PR and other downstream changes that depend on mint 1.3. Thanks!

ericmj commented 3 years ago

Sorry for not getting back to you earlier on this.

The solution to the test server problem is to make it a GenServer. Today the server only sets up the connection and then hands over the socket to the test process. By making it a GenServer that owns the socket instead of handing it over it can handle async frames such as SETTINGS.

Another thing we need to consider here is we need a way to inform the user that the server settings has been received so that they can check them and start honoring what the server requests.

lukebakken commented 2 years ago

@ericmj I've got some time to work on this. In 2630184e21c3eef741732864be3d321e221f161c I've created TestServer2 to try to move the test HTTP2 server into a GenServer. Let me know if it looks like I'm barking up the wrong tree.

I can see how the current implementation of the test server and tests is very synchronous (send data, decode, etc) and moving that to a GenServer isn't quite obvious. Right now I'm just working on the ping / pong tests that have the :wip tag. Suggestions are very welcome!

mix test --only wip test/mint/http2/conn_test2.exs

As always, please feel free to push code changes directly to my branch. I won't be offended :joy_cat:

@mdsebald you may find this interesting.

lukebakken commented 2 years ago

@ericmj @whatyouhide I have this feature completed for now, but there are some open questions and cleanup that remain -

ericmj commented 2 years ago

I removed some very old Erlang and Elixir versions from the GitHub workflows that were missing required functions and had other issues. It didn't feel productive to try and address problems with such old versions. What do you think?

What were the issues with supporting the older versions?

ericmj commented 2 years ago

As you can see here, there are two new responses for the frames associated with settings and a settings ack. This is necessary so that we "know" when these frames are available in HTTP2 tests that require them. This information might also be useful for library users, however, it's a major breaking change. What I thought I'd do next is add a setting to the HTTP2 connection struct to allow a user to specify whether or not to return these responses when recv or stream is called. What do you think?

If we add a setting to do keep it backwards compatible then we also need to keep the old behavior where receiving the settings is async during the connection setup.

lukebakken commented 2 years ago

What were the issues with supporting the older versions?

I remember that start_supervised! wasn't defined in old Elixir versions, and tests using Elixir 1.7.x had issues with the HTTP2 GenServer's port message timing out every single time - https://github.com/elixir-mint/mint/runs/4052328993?check_suite_focus=true. There were other issues as well that you can see in those CI runs.

I would have worked on these issues but building old Erlangs on my Arch Linux dev machine is a giant PITA anymore.

I did try to reproduce the port issue locally using Erlang 22 and Elixir 1.7.4 and couldn't :man_shrugging:

But, if we want to investigate I could use Docker or a VM. Let me know.

lukebakken commented 2 years ago

If we add a setting to do keep it backwards compatible then we also need to keep the old behavior where receiving the settings is async during the connection setup.

Perfect, I'll do that. I'm assuming you meant "sync" rather than "async" there.

I figure I will add a boolean called enable_async_settings to the conn struct that defaults to false.

lukebakken commented 2 years ago

@ericmj - no big rush, I thought I'd let you know that I'm all set for your next review. Thank you!

lukebakken commented 2 years ago

PS I haven't forgotten about this PR! I plan on working on it between now and the new year. Merry Christmas 🎅

lukebakken commented 2 years ago

@ericmj I'm wondering about what you said about :settings and :settings_ack.

ericmj commented 2 years ago

Having those available if wanted does make it possible to have tests for those frames

Can you test them by calling the functions to get the settings instead?

Do you think any libraries (like finch) that use mint could need them? I'm going to check how finch uses mint the next time I have to work on this.

Unsure, but it's hard to design something before you know the use cases. We can always add them later if we find a use case.

lukebakken commented 2 years ago

@ericmj let me know if my recent commits are what you're thinking.

lukebakken commented 2 years ago

@ericmj I'm going to resolve conflicts today or this weekend. Any time for another review?

lukebakken commented 1 year ago

@ericmj howdy! I just updated this branch with the latest changes in main. I see there are a few build failures. Before I dig into them I'm going to see if you're interested in this PR. If not, no worries, I'll close it. I'll check back in a week (October 25th)