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

Add a CloseDeadline function to Connection #181

Merged
merged 5 commits into from
Mar 14, 2023
Merged

Add a CloseDeadline function to Connection #181

merged 5 commits into from
Mar 14, 2023

Conversation

Zerpet
Copy link
Contributor

@Zerpet Zerpet commented Mar 10, 2023

Summary

  • Add CloseDeadline function to Connection
  • Remove unused function in integration tests
  • Updated contribution guidelines

Fixes #178

Background information

During a memory alarm, RabbitMQ will block publisher connections. This means that RabbitMQ will stop reading from the connection socket. In this case, any further attempts from the client using this connection will be blocked on I/O. For example, a connection close request, will block in I/O, and the client application will be blocked.

Other clients, like Java or C#, use a timeout when closing a connection, and return an exception if the connection is not closed within the timeout. Those clients also do connection recovery, but that's outside of the scope of this client (See README anti-goals).

This client now also provides a Connection.CloseDeadline function to specify an I/O timeout for the connection close request to complete.

@Zerpet Zerpet requested a review from lukebakken March 10, 2023 17:59
@Zerpet
Copy link
Contributor Author

Zerpet commented Mar 10, 2023

Did not set a milestone because, following strict-semver, a new public API function must be introduced in a new minor version :-/

@lukebakken lukebakken added this to the 1.7.1 milestone Mar 10, 2023
@lukebakken
Copy link
Contributor

@Zerpet I renamed 1.7.1 to 1.8.0

Zerpet and others added 4 commits March 10, 2023 12:03
A deadline to set an I/O timeout to Connection closure is important,
because RabbitMQ may block the connection (during a memory alarm), and
stop reading from the connection socket. In this case, Connection.Close()
will block on I/O. Whilst closing the connection is not the solution to
the memory alarm, an application should have the ability to close a
connection to a service, regardless of the state of the service.

Signed-off-by: Aitor Perez Cedres <acedres@vmware.com>
Signed-off-by: Aitor Perez Cedres <acedres@vmware.com>
@lukebakken
Copy link
Contributor

@Zerpet let me know if my recent changes are fine.

@Zerpet
Copy link
Contributor Author

Zerpet commented Mar 14, 2023

Yeah, those changes look good 👍 I'm gladly surprised the new test works on GitHub Actions, I thought we didn't have access to docker inside the actions container 🐳

@Zerpet Zerpet merged commit e4711f3 into main Mar 14, 2023
@Zerpet Zerpet deleted the issue-178 branch March 14, 2023 18:04
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.

the publishWithContext interface will not return when it times out
2 participants