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 the client connection after a read error #23

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

uniphil
Copy link

@uniphil uniphil commented Nov 18, 2024

The existing code doesn't defer c.con.Close() after connecting, and exits early if reading encounters any error -- so .Close() is never called on the connection in that case. This change stores any read and closing errors, Joining them on return, ensuring that .Close() is always called.

I'm not 100% sure but I hope this will help prevent the half-closed client connections referenced from #21

There is probably a more idiomatic way to do this (i am a go noob) but if i understand errors.Join and the current code, I think this should keep pretty close to the current intended behavior for any caller.

The existing code doesn't `defer c.con.Close()` and exits early if reading encounters any error -- so `.Close()` is never called on the connection. This change stores any read and closing errors, `Join`ing them on return, ensuring that `.Close()` is always called.

I'm not 100% sure but I hope this will help prevent the half-closed client connections referenced from bluesky-social#21

There is probably a more idiomatic way to do this (i am a go noob) but if i understand `errors.Join` and the current code, I think this should keep pretty close to the current intended behavior for any caller.
@ericvolp12
Copy link
Collaborator

Thanks!

@ericvolp12 ericvolp12 merged commit 4cb3eef into bluesky-social:main Dec 10, 2024
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