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

List amqp10 connections #5881

Merged
merged 11 commits into from
Sep 30, 2022
Merged

List amqp10 connections #5881

merged 11 commits into from
Sep 30, 2022

Conversation

MarcialRosales
Copy link
Contributor

Proposed Changes

This PR fixes issue #4238.

The issue was due to how we searched for the reader process along the supervisor hierarchy tree.
Additionally, we added the missing internal_info method that provided the info item pid which was missing from the reader module.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

in ETS, but not returned by rabbitmqctl
Maybe to fix this issue we can look up the process
in ETC done by rabbit_connection_tracker module.
At the moment, this is one reader process but
when we query its info, the process crashes
calling exactly the function which resolves user item info.

crasher:
 initial call: rabbit_reader:init/3
 pid: <0.769.0>
 exception error: no function clause matching
              rabbit_amqp1_0_reader:info_internal(user,
The issue was related to the supervisor
hierarchy we used to search for the reader
process
@lukebakken lukebakken force-pushed the list-amqp10-connections branch from 5318942 to 78f4aaf Compare September 27, 2022 23:35
Copy link
Collaborator

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

@MarcialRosales thank you for this! I just have one question.

deps/rabbitmq_amqp1_0/src/rabbit_amqp1_0_reader.erl Outdated Show resolved Hide resolved
@lukebakken
Copy link
Collaborator

Testing results look good...

  • Command output is as expected ✔️
    $ ./sbin/rabbitmqctl list_amqp10_connections
    Listing AMQP 1.0 connections ...
    pid
    <[email protected]>
    
  • Viewing the connection from the management UI works as expected ✔️
  • Closing the connection from the management UI works as expected ✔️

@michaelklishin michaelklishin merged commit 1639e3a into main Sep 30, 2022
@michaelklishin michaelklishin deleted the list-amqp10-connections branch September 30, 2022 08:39
michaelklishin added a commit that referenced this pull request Sep 30, 2022
@michaelklishin michaelklishin added this to the 3.10.9 milestone Oct 8, 2022
michaelklishin added a commit that referenced this pull request Oct 8, 2022
List amqp10 connections (backport #5881) (backport #5933)
@ansd
Copy link
Member

ansd commented Sep 12, 2023

This PR introduces 2 new bugs:

  1. It not only lists AMQP 1.0 connections, but also queries AMQP 0.9.1 connections.
  2. Crashes occur when there are AMQP 0.9.1 connections.

To reproduce, run make -C deps/rabbitmq_amqp1_0 ct-command t=non_parallel_tests:when_one_connection FULL=1 with #9290 enabled, or manually create 1 AMQP 0.9.1 connection and run rabbitmqctl list_amqp10_connections and check the RabbitMQ log file:

crasher:
  initial call: rabbit_reader:init/3
  pid: <0.652.0>
  registered_name: []
  exception exit: {unexpected_message,{info,[pid],<0.707.0>}}
    in function  rabbit_reader:handle_other/2 (rabbit_reader.erl, line 640)
    in call from rabbit_reader:mainloop/4 (rabbit_reader.erl, line 531)
    in call from rabbit_reader:run/1 (rabbit_reader.erl, line 453)
    in call from rabbit_reader:start_connection/5 (rabbit_reader.erl, line 352)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants