[STAFF-31] Add proper shutdown handling to Amqpx generic producers and consumers #138
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
which turns off the channel normally when reason is
`:normal`, `:shutdown`, or `{:shutdown, term}`
claudio-dalicandro
requested review from
cando,
luca-tansini,
mbusi and
emilianobovetti
Comment on lines
+184
to
+188
| case reason do | ||
| :normal -> close_channel(channel) | ||
| :shutdown -> close_channel(channel) | ||
| {:shutdown, _} -> close_channel(channel) | ||
| _ -> :ok |
There was a problem hiding this comment.
shouldn't we close the channel regardless of the reason? 🤔
There was a problem hiding this comment.
Actually, the channel is always closed because it is linked. However, for "normal" closures, we need to add some logic, since as you can see, amqp_client does not consider {:shutdown, term} or simply :shutdown as normal closures
| @@ -26,10 +26,11 @@ defmodule Amqpx.Gen.Consumer do | |||
|
|
|||
| @gen_server_opts [:name, :timeout, :debug, :spawn_opt, :hibernate_after] | |||
|
|
|||
| @spec start_link(opts :: map()) :: GenServer.server() | |||
There was a problem hiding this comment.
Oops... Maybe here a spec can help :D
luca-tansini
approved these changes
Mar 15, 2023
I've only replicated the previous behavior, but I suppose it's because a consumer usually interacts with others, and it's rare for others to ask things of them, while a producer is typically the one being contacted |
matteosister
approved these changes
Mar 16, 2023
claudio-dalicandro
requested review from
cottinisimone
and removed request for
cando
cottinisimone
deleted the
STAFF-31/spike/graceful-shutdown-for-consumers
branch
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the Amqpx producers and consumers did not handle shutdown gracefully, resulting in noisy log messages and a strange behavior when the supervisor attempted to shut them down.
To address this issue and to support use cases where the actors need to be shut down gracefully, I have added a new
terminatefunction to the actors implementation. It gracefully shuts down the associated channels when reasons like:normal,:shutdown, and{:shutdown, reason :: term()}are encountered.This change also enables the use of the library in distributed environments, where the actors may need to be shut down deliberately, such as when balancing nodes in a cluster.