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

Should we catch signals that could cause semaphores to leak? #856

Open
jbaublitz opened this issue May 17, 2023 · 1 comment
Open

Should we catch signals that could cause semaphores to leak? #856

jbaublitz opened this issue May 17, 2023 · 1 comment

Comments

@jbaublitz
Copy link
Member

We recently bumped into a problem where stratisd was leaking semaphores on SIGTERM. Should we add in signal handling for the semaphore code?

@bmr-cymru @mulkieran Curious on your thoughts.

@bmr-cymru
Copy link
Member

Generally it's more common for libraries to leave signal handling to the calling application - e.g. this is what happens in lvm2/libdevmapper. Whenever lvm2 is holding a VG lock it blocks signals (this encompasses DM device activation as well as things like metadata updates). If we followed that approach in Stratis then stratisd would block signals before operating on DM devices.

Alternately we could try to handle this internally in devicemapper but give users the ability to opt out if they want to do their own signal handling. I think that's possible and would mean changing UdevSync to block/unblock signals around operations. This could be gated by a feature for clients that don't want that behaviour.

The only thing I'm uncertain of is how this will interact with threading - if I remember correctly blocked signals are per-thread, so by blocking signals in the thread that's processing a DM operation we'd still have the possibility of SIGTERM/SIGINT being delivered to another thread in the process. I'm not certain what would happen in this case and whether we'd need to block signals in other threads (or in the main thread before starting other threads) to prevent an abrupt termination of stratisd.

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

No branches or pull requests

2 participants