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

Incorrect handling of signal handlers #295

Closed
mcopik opened this issue Dec 7, 2021 · 6 comments · Fixed by #296
Closed

Incorrect handling of signal handlers #295

mcopik opened this issue Dec 7, 2021 · 6 comments · Fixed by #296
Labels
bug Something isn't working good first issue an issue suitable to start contributing to the project

Comments

@mcopik
Copy link

mcopik commented Dec 7, 2021

Hi!

I'm experiencing the same problem as issues #187 and #188. As soon as I start the server, Crow steals signals handlers even if I call signal_clear before. It was very confusing, but I was able to dig into the library's code and find out the source. In fact, I think it's caused by two separate issues:

(1) I use SSL App and the SSL implementation doesn't clear the signals in the server implementation at all. - app.h, line 279 I manually added the code from the non-SSL version. This helped partially - Crow didn't register signals, but it still overrode them and I had no signals at all after running run.

(2) Finally, I was able to find out that just creating the signal_set from Boost Asio affects OS signal handling. And since they are created inside the constructor of http_server, which instance is dynamically allocated in run, it will clear SIGINT but not start a handler. I fixed the issue for myself by changing the constructor to allocating an empty set - maybe it could be a generic solution if signal handlers are always added anyway by entities using this class?

@The-EDev The-EDev added bug Something isn't working good first issue an issue suitable to start contributing to the project labels Dec 8, 2021
@The-EDev
Copy link
Member

The-EDev commented Dec 8, 2021

Interesting find, Thanks a lot.

  1. Seems like an oversight while working on [fix] Make signal handler optional #85, Thanks for pointing it out.
  2. Not exactly an oversight, but the code wasn't changed when [fix] Make signal handler optional #85 was added.

Thanks again for the detailed description, both issues will be fixed promptly.

@samichoulo911
Copy link

samichoulo911 commented Dec 10, 2021

I am having a similar issue where signals were still being intercepted after server.stop() was called. Is there a reason why the stop() method doesn't call signal_clear() ?

@The-EDev
Copy link
Member

@samichoulo911 if you're calling server.stop() on your own you should probably clear the signals before you start the application, at least when it comes to normal operations.

The reason stop() doesn't clear the signals is that the Crow app was not designed to be manipulated after calling run(). Therefore the effects of doing so (if any) are unknown at this time.
You do bring up a good point in clearing the signals and I do want to look into it. But considering it's more of a feature rather than a bug, I do believe it is outside this issue's scope.

@samichoulo911
Copy link

Thank for the explanation! When poking around the code trying to see who was using the signals I noted that the run() method calls signal_clear() right before adding them back to signals (see image). This is most probably out of this issue's scope, but I was just curious to know the reasoning behind.
image

@The-EDev
Copy link
Member

I believe this is because (prior to #296) the server's constructor would use default signals SIGINT and SIGTERM, so the app would need to clear those before adding custom signals.

@samichoulo911
Copy link

This clears up my confusion thanks!

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue an issue suitable to start contributing to the project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants