Skip to content

MQTT and Streams: handle connection shutdown via CLI command gracefully #12118

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.

Sign up for GitHub

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

Jump to bottom
Merged
dcorbacho merged 4 commits into from
Aug 28, 2024
Merged

MQTT and Streams: handle connection shutdown via CLI command gracefully #12118

dcorbacho merged 4 commits into from
Aug 28, 2024

Conversation

@dcorbacho
Copy link
Contributor

@dcorbacho dcorbacho commented Aug 25, 2024

{shutdown, Reason} must be handled into handle_call and not handle_info rabbitmqctl close_all_user_connections calls rabbit_reader which does a call into the process, the same as rabbitmq_management

Fixes #11985

@dcorbacho dcorbacho requested review from ansd and mkuratczyk August 25, 2024 21:44
@michaelklishin michaelklishin modified the milestone: 3.13.8 Aug 26, 2024
ansd
ansd requested changes Aug 26, 2024
View reviewed changes
Copy link
Member

@ansd ansd left a comment

Choose a reason for hiding this comment

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

  1. This PR introduces a crash as can be seen in test case management_plugin_connection in the shared_SUITE.
    For example running
make -C deps/rabbitmq_mqtt ct-shared t=[mqtt,cluster_size_1,v5]:management_plugin_connection

will make the MQTT connection crash with:

2024-08-26 08:47:10.764564+02:00 [error] <0.1022.0> ** Reason for termination ==
2024-08-26 08:47:10.764564+02:00 [error] <0.1022.0> ** {mqtt_unexpected_msg,{shutdown,"Closed via management plugin"}}

  1. This PR doesn't handle WebMQTT connections.

  2. It would be nice to add a test case for this change.

  3. Ideally, we refactor such that closing connections via the Management API and via the CLI command both cause the same logic to happen in RabbitMQ, i.e. cause either cast or call for (Web)MQTT connections.

mkuratczyk
mkuratczyk reviewed Aug 26, 2024
View reviewed changes
Copy link
Contributor

@mkuratczyk mkuratczyk left a comment

Choose a reason for hiding this comment

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

Thanks. This solves the original problem with the exception, but I now noticed this in the logs:

2024-08-26 08:53:40.395974+02:00 [info] <0.988.0> Closing connection <0.918.0> because <<"foobar">>
2024-08-26 08:53:40.396062+02:00 [info] <0.918.0> MQTT closing connection <<"127.0.0.1:54961 -> 127.0.0.1:1883">>: <<"foobar">>
2024-08-26 08:53:40.396161+02:00 [warning] <0.988.0> Could not close connection <0.918.0> (reason: <<"foobar">>): {shutdown,
2024-08-26 08:53:40.396161+02:00 [warning] <0.988.0>

This warning is unexpected

@ansd
Copy link
Member

ansd commented Aug 26, 2024 •
edited
Loading

Good spot. That's because
5. This PR returns {stop,Reason,NewState} instead of {stop,Reason,Reply,NewState}.

@dcorbacho
Copy link
Contributor Author

dcorbacho commented Aug 26, 2024

Thanks. I've just seen the issue. rabbitmq_management calls rabbit_networking:close_connection as expected for network connections, but on the same case sends a message for everything else :( https://github.com/rabbitmq/rabbitmq-server/blob/main/deps/rabbitmq_management/src/rabbit_mgmt_wm_connection.erl#L101

@dcorbacho
Copy link
Contributor Author

dcorbacho commented Aug 26, 2024

@ansd I've restored the original handle_info clause. Changing the management to perform a call when forcing close the connections would be a breaking change for mqtt and streams, it would require a feature flag for mixed-version clusters. Not sure it is worth it.

@dcorbacho
mqtt: handle connection shutdown
494c1b8 
`{shutdown, Reason}` must be handled into handle_call and not handle_info
`rabbitmqctl close_all_user_connections` calls rabbit_reader which does
a call into the process, the same as rabbitmq_management
@dcorbacho dcorbacho force-pushed the issue-11985 branch 3 times, most recently from ef7b12b to 342f29d Compare August 27, 2024 14:42
@dcorbacho dcorbacho requested review from ansd and mkuratczyk August 28, 2024 06:05
mkuratczyk
mkuratczyk approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@mkuratczyk mkuratczyk left a comment

Choose a reason for hiding this comment

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

Functionality-wise I see no problems now, thanks. I'll let David comment on the implementation.

@ansd ansd changed the title mqtt: handle connection shutdown MQTT and Streams: handle connection shutdown gracefully Aug 28, 2024
ansd added 2 commits August 28, 2024 13:18
@ansd
Simplify test cases
69d407e 
1. Only run the CLI tests on a single node cluster. The shared_SUITE is
   already very big. Testing the same CLI commands against node-0 on a
   3-node cluster brings no benefit.
2. Move the two new CLI test cases in front of
   management_plugin_connection because they are similar in that all
   three tests close the MQTT connection.
3. There is no need to query the HTTP API for the two new CLI test
   cases.
4. There is no need to set keepalive in the two new CLI test cases.
@ansd
Avoid crash in stream connection
8c905b9 
1.
Prior to this commit, closing a stream connection via:
```
./sbin/rabbitmqctl close_all_user_connections guest enough
```
crashed the stream process as follows:
```
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>   crasher:
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>     initial call: rabbit_stream_reader:init/1
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>     pid: <0.1098.0>
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>     registered_name: []
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>     exception error: no function clause matching
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>                      rabbit_stream_reader:open({call,
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>                                                 {<0.1233.0>,
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>                                                  #Ref<0.519694519.1387790337.15898>}},
2024-08-28 13:00:18.969931+02:00 [error] <0.1098.0>                                                {shutdown,<<"enough">>},
```

This commit fixes this crash.

2.
Both CLI commands and management plugin use the same way
to close MQTT, Web MQTT, and Stream connections: They all send a message
via `Pid ! {shutdown, Reason}` to the connection.

3.
This commit avoids making `rabbit` core app to know about
'Web MQTT'.

4
This commit simplifies rabbit_mqtt_reader by avoiding another
handle_call clause
@ansd
Copy link
Member

ansd commented Aug 28, 2024

Thank you @dcorbacho.
I noticed that Stream connections also crash.
I pushed two commits which simplify a lot (see commit messages for details) and fix the Stream connection crash as well (manually tested).

@ansd ansd changed the title MQTT and Streams: handle connection shutdown gracefully MQTT and Streams: handle connection shutdown via CLI command gracefully Aug 28, 2024
@dcorbacho dcorbacho merged commit afa28cb into main Aug 28, 2024
@dcorbacho dcorbacho deleted the issue-11985 branch August 28, 2024 12:27
@mergify mergify bot mentioned this pull request Aug 28, 2024
Merged
@mergify mergify bot mentioned this pull request Aug 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkuratczyk mkuratczyk mkuratczyk approved these changes

@ansd ansd ansd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unhandled exceptions are logged when closing MQTT or Stream connections via CLI

5 participants

@dcorbacho @ansd @mkuratczyk @michaelklishin