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

Call Async Callback to Correctly Initiate Shutdown #1637

Conversation

@ChristopherDavenport
Copy link
Member

@ChristopherDavenport ChristopherDavenport commented Jan 23, 2018

Fixes Concurrently Leakage

I don't know why we were even using async here originally but by not calling the provided callback we broke lots of stuff. Lesson for the day, don't do that.

Copy link
Member

@jmcardon jmcardon left a comment

Definitely obvious once you see the diff, but before this, without remembering the signature of async, this was pretty hard to spot.

Great stuff @ChristopherDavenport

@ChristopherDavenport ChristopherDavenport merged commit 29671c0 into http4s:master Jan 23, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ChristopherDavenport ChristopherDavenport deleted the ChristopherDavenport:callAsyncCallbackToFixConcurrently branch Jan 23, 2018
@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 23, 2018

Does serve still block until interrupted? That was the original intent. I'm afraid this will make Stream apps terminate immediately.

@ChristopherDavenport
Copy link
Member Author

@ChristopherDavenport ChristopherDavenport commented Jan 23, 2018

No. It terminates now immediately. Making an issue and trying to work through it this morning.

The problem is that it doesn't block until interrupted in the original implementation, it just blocks forever.

@aeons
Copy link
Member

@aeons aeons commented Jan 23, 2018

I "fixed" this in the initial transition to cats as well, (stupider though :)) in #1266

And reverted it in #1334

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.