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

Close connections immediately (when we know that we can do that) #493

Merged
merged 2 commits into from
Feb 14, 2020
Merged

Conversation

neongreen
Copy link

Fixes #490.

I'm going to test this in https://github.com/wireapp/wire-server so please don't merge that yet. @LeifW @naushadh @dsturnbull would you also like to take a look?

@neongreen
Copy link
Author

So there's a choice of two commits now:

  • one does sinkNull
  • the other closes the connections without sinking anything

The latter is several times faster in my benchmarks (five times faster for SQS's deleteMessage with local fake SQS), but I'm not sure it's correct. On the other hand, I also don't see any reason why it wouldn't be correct, but it still seems suspicious.

@neongreen neongreen changed the title Add 'sinkNull' to close connections when we know that we can do that Close connections immediately (when we know that we can do that) Oct 1, 2018
@naushadh
Copy link
Contributor

naushadh commented Oct 1, 2018

I too would prefer the faster way, correctness provided. Perhaps long term we really do need some actual testing against AWS (or a mock of it) (#477) and/or lift connection handling into the type system.

@neongreen
Copy link
Author

This is in production in https://github.com/wireapp/wire-server now and things haven't broken yet, so I'm in favor of merging it. cc @MichaelXavier?

@dzhus
Copy link

dzhus commented Jun 12, 2019

Same here – seems ok to merge

@mheinzel
Copy link
Contributor

I rebased this change, which we have been running in production in https://github.com/wireapp/wire-server for over a year at this point.

Some CI builds failed because of happy, one because of "No output has been received in the last 10m0s", but this seems to happen on master, too.

If there is any interest in merging this, that would be great. If not, we'll keep using our fork.
Let me know if there is anything I could do to help make this happen.

@brendanhay
Copy link
Owner

LGTM

@brendanhay brendanhay merged commit 9cf5b57 into brendanhay:develop Feb 14, 2020
@mheinzel
Copy link
Contributor

Wow, thanks for the quick response!

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.

Open connections are leaking
5 participants