Add defensive connection closure. #328
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lukasa
force-pushed
the
cb-leaking-connections-is-bad
branch
3 times, most recently
from
07b6889 to
459efe1
Compare
artemredkin
approved these changes
Jan 19, 2021
Lukasa
force-pushed
the
cb-leaking-connections-is-bad
branch
2 times, most recently
from
63f6858 to
13d82d1
Compare
weissi
reviewed
Jan 19, 2021
| // we close now to cover our backs. We don't care about the result: if the channel is | ||
| // _already_ closed, that's fine by us. | ||
| if closing { | ||
| logger.debug("closing connection") |
There was a problem hiding this comment.
I'm totally fine with this but the information here is redundant (because we log it above (metadata: ["ahc-closing": "\(closing)"]).
There was a problem hiding this comment.
@Lukasa whilst we're here, do you recall if there is metadata attached to the logger still here? Ie. can this message be correlated with the request that made the close happen? I think that would be good.
There was a problem hiding this comment.
Yes, it does: we never remove the metadata key from this logger. We create a new logger when a new request is minted.
Motivation: Currently when either we or the server send Connection: close, we correctly do not return that connection to the pool. However, we rely on the server actually performing the connection closure: we never call close() ourselves. This is unnecessarily optimistic: a server may absolutely fail to close this connection. To protect our own file descriptors, we should make sure that any connection we do not return the pool is closed. Modifications: If we think a connection is closing when we release it, we now call close() on it defensively. Result: We no longer leak connections when the server fails to close them. Fixes swift-server#324.
Lukasa
force-pushed
the
cb-leaking-connections-is-bad
branch
from
13d82d1 to
52a44ba
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Currently when either we or the server send Connection: close, we
correctly do not return that connection to the pool. However, we rely on
the server actually performing the connection closure: we never call
close() ourselves. This is unnecessarily optimistic: a server may
absolutely fail to close this connection. To protect our own file
descriptors, we should make sure that any connection we do not return
the pool is closed.
Modifications:
If we think a connection is closing when we release it, we now call
close() on it defensively.
Result:
We no longer leak connections when the server fails to close them.
Fixes #324.