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

Remove the key from idleQueue if it is empty after dequeue #1922

Merged
merged 3 commits into from Jun 15, 2018

Conversation

Projects
None yet
4 participants
@tbrown1979
Contributor

tbrown1979 commented Jun 14, 2018

Should fix #1920

@rossabaker

I think this works. We could alternatively use getConnectionFromQueue(randKey), which perserves the non-empty idle queue invariant, and log a warning if we get None.

@SystemFw

This comment has been minimized.

Contributor

SystemFw commented Jun 14, 2018

Not really asking for this specific PR, but a few more comments explaining the design of the Manager (don't have to be detailed, even a general design comment at the top) would go a long way given that the code is slightly hairier than average http4s :)

@tbrown1979

This comment has been minimized.

Contributor

tbrown1979 commented Jun 14, 2018

@rossabaker That's a great point. I can make that change.

@SystemFw That would definitely be nice. Fwiw, once cats-effect 1.0 is released I can work on purifying the PoolManager which would make it a lot easier to reason about.

@tbrown1979

This comment has been minimized.

Contributor

tbrown1979 commented Jun 14, 2018

@rossabaker Let me know if that looks okay to you.

@rossabaker

👍

As for future work, I think this needs a refactoring anyway to take advantage of cancellation. Would be a good opportunity to see what the cats-effect concurrency tools might buy us.

@tbrown1979

This comment has been minimized.

Contributor

tbrown1979 commented Jun 15, 2018

@rossabaker I look forward to seeing how I can improve things with the new cats-effect tools 😄

@SystemFw

This comment has been minimized.

Contributor

SystemFw commented Jun 15, 2018

@tbrown1979 You can start working on it right now probably, cats-effect 1.0 is not out yet but I don't foresee big changes from now until the release. Feel free to ping me should you need a hand with the new concurrency stuff there.

@rossabaker Maybe that's a sufficient knock on the door for a milestone?

@ChristopherDavenport

Does this actually work as expected still then? As now we don't actually remove an idle connection do we? Meaning we could end up increasing connections?

I'm bad at imperative code now, so I apologize.

@tbrown1979

This comment has been minimized.

Contributor

tbrown1979 commented Jun 15, 2018

@ChristopherDavenport We still do remove an idle connection here. The getConnectionFromQueue will .dequeue iff the Queue for the given key is .nonEmpty, and then it returns us that connection. We'll then .shutdown() that connection. Let me know if that helps!

edit: And as Ross said this preserves the invariant that a Queue in the idleQueue should not be empty. It should either have >= 1 elements or not be in the Map

@tbrown1979

This comment has been minimized.

Contributor

tbrown1979 commented Jun 15, 2018

Should I make the log I added an error instead of warn? It's something that should never happen, and if it does it might be good to have it louder than warn?

@ChristopherDavenport

This comment has been minimized.

Member

ChristopherDavenport commented Jun 15, 2018

Yet we have experience it does. So I am inclined towards the level it is at.

@ChristopherDavenport ChristopherDavenport merged commit 561171c into http4s:release-0.18.x Jun 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tbrown1979 tbrown1979 deleted the tbrown1979:fix-pool-mgr-bug-queue-empty branch Sep 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment