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

Fix async http adapter #901

Merged
merged 1 commit into from Sep 11, 2020
Merged

Conversation

bruno-
Copy link
Contributor

@bruno- bruno- commented Sep 8, 2020

Problem: the server in Async::HTTP::WebMockEndpoint#accept_socket was
hanging forever after the rspec example finished.
This is normal as Async::HTTP::Server (or any server) is never supposed
to end. It just continues waiting for new requests which, in this case,
never come.

Solution: use a 'transient' async task when starting a server.
Transient tasks do not block an exiting async reactor.
So, in this case a transient server will not hang forever.

Fixes #858

@ioquatix we talked about making this a draft PR so we can continue talking about a proper webmock <-> async-http solution. Since this fix is minimal I propose going for a merge.
We can continue thinking about a better approach here socketry/async-http#35

Problem: the server in Async::HTTP::WebMockEndpoint#accept_socket was
hanging forever after the rspec example finished.
This is normal as Async::HTTP::Server (or any server) is never supposed
to end. It just continues waiting for new requests which, in this case,
never come.

Solution: use a 'transient' async task when starting a server.
Transient tasks do not block an exiting async reactor.
So, in this case a transient server will not hang forever.

Fixes bblimke#858
@bruno-
Copy link
Contributor Author

bruno- commented Sep 8, 2020

@ioquatix I just saw your latest comment #858 (comment)

Here is the "expanded" code from this PR:

Async(transient: true) do  # <= This task IS the server's task
  server = Async::HTTP::Server.new(WebMockApplication, self)
  server.accept(socket, socket.remote_address)
end

Is the ensure block with task&.stop necessary? If yes, we'd need to insert another task.async do... between Async and Async::HTTP::Server.new in the code above.

@ioquatix
Copy link
Contributor

ioquatix commented Sep 8, 2020

It looks okay to me - let's try it as implemented and check how it works.

@bblimke
Copy link
Owner

bblimke commented Sep 11, 2020

@bruno- @ioquatix thank you both for investigating this and for the fix.

@bblimke bblimke merged commit 8ce537a into bblimke:master Sep 11, 2020
@bblimke
Copy link
Owner

bblimke commented Sep 11, 2020

@bruno- @ioquatix actually the build is now failing: https://travis-ci.org/github/bblimke/webmock/jobs/725160832
ArgumentError: unknown keyword: transient

@bruno-
Copy link
Contributor Author

bruno- commented Sep 11, 2020

I'm checking this.
My guess is webmock is using an older version of async or async-http gems that don't support that keyword.

@bruno-
Copy link
Contributor Author

bruno- commented Sep 11, 2020

@bblimke it seems travis is using async 1.24.2 https://travis-ci.org/github/bblimke/webmock/jobs/725160832#L626
That version is from February this year, while transient feature was introduced in April.
async-http version >= 0.48.0 specified in webmock gemfile is fine. I don't think that needs to be changed.

Can we bump travis version? Maybe delete travis' cache or something?

@bruno- bruno- deleted the fix_async_http_adapter branch September 11, 2020 12:40
@bblimke
Copy link
Owner

bblimke commented Sep 11, 2020

@bruno- I now see that it's only a problem on Ruby 2.3 and 2.4 which are likely not compatible with latest async version.

2.3 and 2.4 are EOL so I guess I can drop support for them anyway.

@ioquatix
Copy link
Contributor

I have an unpublished policy for open source, I do not support EOL Rubies.

@bruno-
Copy link
Contributor Author

bruno- commented Sep 12, 2020

@bblimke looking forward to next webmock gem release that contains these fixes.

@bblimke
Copy link
Owner

bblimke commented Sep 13, 2020

This has now been released as version 3.9.0

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.

Async:HTTP never returns
3 participants