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

chore(accept_server): optionally print stack trace on signal #156

Closed

Conversation

kostasrim
Copy link
Contributor

Allow to optionally print stack traces on received signals

@codecov-commenter
Copy link

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (e0987ee) 77.34% compared to head (6e66704) 77.26%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
- Coverage   77.34%   77.26%   -0.08%     
==========================================
  Files          98       98              
  Lines        7173     7192      +19     
==========================================
+ Hits         5548     5557       +9     
- Misses       1625     1635      +10     
Files Coverage Δ
util/fibers/accept_server.cc 78.40% <40.00%> (-3.81%) ⬇️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@romange
Copy link
Owner

romange commented Oct 13, 2023

There are may be several non-intrusive options.

  1. Why not send "DEBUG STACKTRACE" before shutting down the process?
  2. There is also ListenerInterface::PreShutdown hook which may help with what you need.

@kostasrim
Copy link
Contributor Author

kostasrim commented Oct 13, 2023

Isn't it the case that we want to print the stacktraces only when instance.stop() fails in pytests? There we use SIGKILL which brutally kills the process, so I don't think any of the two above get a chance to run? That's why I wanted to use a signal, such that before we proc.kill() I send a proc.send_signal(signal.SIGINT) to allow DF print the stacktraces before it gets brutally killed. (SIGKILL can't be handled or blocked)

Is there something I am missing?

@romange
Copy link
Owner

romange commented Oct 13, 2023 via email

@kostasrim
Copy link
Contributor Author

But you can register for another signal in dragonfly via a proactor. Does not have to be sigint.

Yep that's true and now I am thinking about this doesn't need to be done in helio.

@kostasrim
Copy link
Contributor Author

kostasrim commented Oct 13, 2023

Another thing, accessing server logs is the prerequisite for this pr, because I am not sure the system is stable enough at this stage to actually execute the stacktrace printing logic.

I agree, this might never happen and it's kinda a longshot. But then what's the value of printing stacktraces then? We want to print them when things go bad such that we can use them to figure out what happened (for cases that the python code must brutally kill DF via proc.kill() because it doesn't respond).

Also regular shutdown is impossible, if we reached proc.kill then we don't really have many options. The only room is to issue a signal before we kill and hope that it actually gets processed.

Another thing that would add value here is the core dump...

All in all, given that the system is unstable and that the signal might not actually print the stacktraces, is there really added value by this idea?

What's your opinion?

@kostasrim kostasrim closed this Oct 18, 2023
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.

3 participants