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

Ensure all connections are closed regardless of exception #254

Merged
merged 4 commits into from
Mar 7, 2024

Conversation

glebvsmir
Copy link

@glebvsmir glebvsmir commented Mar 6, 2024

Hello,
With some recent changes made that support various types of exceptions, including cancellation, it turned out that some connections were not being closed leading to a leak of connections.
The solution is to add a nested try...finally code block to ensure that connections are always closed for all possible exceptions.

@glebvsmir glebvsmir marked this pull request as ready for review March 6, 2024 02:00
@Lanayx
Copy link
Member

Lanayx commented Mar 6, 2024

Thanks for commit! I think that the change can be simplified just by padding post operationsMb ChannelInactive 4 spaces left, so it will be called regardless of exception

@glebvsmir
Copy link
Author

Thanks for commit! I think that the change can be simplified just by padding post operationsMb ChannelInactive 4 spaces left, so it will be called regardless of exception

Simplified as you recommended. This has been re-tested and appears to behave the same under normal execution.

@Lanayx
Copy link
Member

Lanayx commented Mar 6, 2024

I meant only 4 spaces left :) But you can leave it with 8 spaces left, in that case can you please also remove post operationsMb ChannelInactive from line 819 since it will be called twice

@glebvsmir
Copy link
Author

I meant only 4 spaces left :) But you can leave it with 8 spaces left, in that case can you please also remove post operationsMb ChannelInactive from line 819 since it will be called twice

Good observation. Updated per your original suggestion to keep connection closing within the exception handling block. I prefer to leave the "else" behavior as is.

@glebvsmir glebvsmir changed the title Ensure all connections are closed using finally Ensure all connections are closed regardless of exception Mar 7, 2024
@Lanayx Lanayx merged commit fe19ec5 into fsprojects:2.x Mar 7, 2024
2 checks passed
@Lanayx
Copy link
Member

Lanayx commented Mar 7, 2024

@glebvsmir
Copy link
Author

Thank you

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.

2 participants