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() closes immediately; Add new closeGracefully() #383

Merged
merged 6 commits into from
Aug 10, 2023

Conversation

fabianfett
Copy link
Collaborator

@fabianfett fabianfett commented Aug 6, 2023

Fixes #370.

This PR changes the behavior of close(). close() now closes a connection immediately. We introduce a closeGracefully() that now has the same behavior as close before. Since we never documented the old close behavior and it is unfortunate that close does depend on the remote server to fulfill all queries, we consider it okay to change the behavior in this way. The current behavior is harmful since there is currently no way to force close a connection. Users might be in a position to force close a connection if a server stops responding.

Alternative: close keeps graceful close behavior and we add a closeForcefully method instead

@fabianfett fabianfett changed the title close() closes immediately; Add new gracefulClose() close() closes immediately; Add new gracefulClose() Aug 6, 2023
@fabianfett fabianfett changed the title close() closes immediately; Add new gracefulClose() close() closes immediately; Add new closeGracefully() Aug 6, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2023

Codecov Report

Merging #383 (ecdb6f5) into main (a5758b0) will increase coverage by 1.46%.
The diff coverage is 71.31%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
+ Coverage   46.07%   47.53%   +1.46%     
==========================================
  Files         109      109              
  Lines        8728     8764      +36     
==========================================
+ Hits         4021     4166     +145     
+ Misses       4707     4598     -109     
Files Changed Coverage Δ
Sources/PostgresNIO/New/PSQLEventsHandler.swift 72.41% <ø> (ø)
Sources/PostgresNIO/Postgres+PSQLCompat.swift 0.00% <0.00%> (ø)
Sources/PostgresNIO/New/PSQLError.swift 79.02% <66.66%> (+5.18%) ⬆️
...nection State Machine/ConnectionStateMachine.swift 58.59% <70.00%> (+2.90%) ⬆️
...es/PostgresNIO/Connection/PostgresConnection.swift 41.11% <85.71%> (+6.51%) ⬆️
.../Connection State Machine/ListenStateMachine.swift 69.03% <87.50%> (+0.57%) ⬆️
...urces/PostgresNIO/New/PostgresChannelHandler.swift 78.21% <100.00%> (+5.94%) ⬆️

... and 5 files with indirect coverage changes

@fabianfett fabianfett requested a review from gwynne August 9, 2023 21:31
@fabianfett fabianfett marked this pull request as ready for review August 9, 2023 21:31
@fabianfett fabianfett added the semver-minor Adds new public API. label Aug 9, 2023
Comment on lines +57 to +60
@available(*, deprecated, renamed: "clientClosesConnection")
public static let connectionQuiescing = Self.clientClosesConnection

@available(*, deprecated, message: "Use the more specific `serverClosedConnection` or `clientClosedConnection` instead")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🚨 @gwynne

Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

I concur that the change in behavior is worthwhile.

@fabianfett fabianfett merged commit 52d5636 into vapor:main Aug 10, 2023
@fabianfett fabianfett deleted the ff-close branch August 10, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PostgresConnection.close() should just close the connection
3 participants