Skip to content

Conversation

vonzshik
Copy link
Contributor

Fixes #5994

@vonzshik vonzshik requested a review from roji as a code owner January 14, 2025 13:44
try
{
// Send close_notify TLS alert to correctly close connection on postgres's side
sslStream.ShutdownAsync().GetAwaiter().GetResult();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the comment above ("This method doesn't actually perform any meaningful I/O, and therefore is sync-only.") is still completely accurate, and whether at some point we should consider having a CleanupAsync...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, certainly. But given that close_notify is just a 60 byte packet, we can probably still consider it as not being meaningful (at least for now).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agree it's unlikely to be an actual problem anywhere... But maybe we should do a proper async variant at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the documentation for the method and opened #6012 to track changing the method to be completely async.

@vonzshik vonzshik enabled auto-merge (squash) February 4, 2025 11:33
@vonzshik vonzshik merged commit e8664e5 into main Feb 4, 2025
13 checks passed
@vonzshik vonzshik deleted the 5994-close_notify-tls-alert branch February 4, 2025 11:41
vonzshik added a commit that referenced this pull request Feb 4, 2025
vonzshik added a commit that referenced this pull request Feb 4, 2025
@vonzshik
Copy link
Contributor Author

vonzshik commented Feb 4, 2025

Backported to 9.0.3 via 10cf87c, 8.0.6 via db11575

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.

Send close_notify tls alert on connection cleanup
2 participants