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

Fix/detach handlers on termination #9

Merged

Conversation

contested-space
Copy link
Contributor

This PR ensures that the event handlers are detached when Peep terminates.

Copy link
Owner

@rkallos rkallos left a comment

Choose a reason for hiding this comment

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

Thank you!

I have a small suggestion about the test to make it more robust to unit tests running in parallel.


{:ok, pid} = GenServer.start(Peep, options, name: options.name)

assert length(:telemetry.list_handlers([])) == 1
Copy link
Owner

Choose a reason for hiding this comment

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

I'm worried this could fail when tests are run in parallel.

I propose two options:

  1. Get the handler id using :sys.get_state(pid), and assert that there is a handler with that id in :telemetry.list_handlers([])
  2. Create a globally unique metric name, and assert that :telemetry.list_handlers(MetricName) == 1 when the Peep process is running and == 0 when the Peep process is terminated.

@contested-space contested-space force-pushed the fix/detach-handlers-on-termination branch from 2d2fd7a to 7d19b13 Compare January 18, 2024 19:02
@rkallos rkallos merged commit f67e72b into rkallos:main Jan 18, 2024
1 check passed
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.

2 participants