fix: Signal Handlers on Non-Windows Platforms in sslclient

Nidhoegger commented 4 months ago

Only set signals to Ignore on non-Linux of no signal-handler is in place. Also set the SA_RESTART flag to avoid operations returning EINTR.

Code change checklist

[x] I have ensured that all methods and functions are fully documented using doxygen style comments.
[x] My code follows the coding style guide.
[x] I tested that my change works before raising the PR.
[x] I have ensured that I did not break any existing API calls.
[x] I have not built my pull request using AI, a static analysis tool or similar without any human oversight.

CLAassistant commented 4 months ago

All committers have signed the CLA.

netlify[bot] commented 4 months ago

Deploy Preview for dpp-dev ready!

Name	Link
Latest commit	1ea7c12452ec7c3ebf291cf40c383e342d6ec086
Latest deploy log	https://app.netlify.com/sites/dpp-dev/deploys/662d3e8f400af4000898751d
Deploy Preview	https://deploy-preview-1123--dpp-dev.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Jaskowicz1 commented 4 months ago

Thanks for your contribution! This PR doesn't seem like it's carried anything from master so it's okay to stay up but next time please make your changes on a branch based from dev, rather than using the master branch!

Jaskowicz1 commented 4 months ago

Also, next time, please read the description of the PR and choose the template that accurately reflects your PR. I've added the template in for you this time, please fill it out (and correct your PR title to follow our conventions please!)

Nidhoegger commented 4 months ago

Since not seeing this on GitHub directly, I will reply here and hope it works. IIRC SIGHUP is generated when the controlling TTY is lost without prior detaching (earlier it was called daemonization iirc). Today, frameworks like runit or launchd are used for daemonization of services, so if SIGHUP is generated, the controlling terminal is closed (most likely) and thus the process will die anyway. So IMHO it does not change anything if SIGHUP is ignored or not.

Am Sa., 6. Apr. 2024 um 18:12 Uhr schrieb Craig Edwards (Brain) < @.***>:

@.**** commented on this pull request.

In src/dpp/sslclient.cpp https://github.com/brainboxdotcc/DPP/pull/1123#discussion_r1554634258:

@@ -237,11 +249,11 @@ ssl_client::ssl_client(const std::string &_hostname, const std::string &_port, b keepalive(reuse) {

ifndef WIN32

signal(SIGALRM, SIG_IGN);

signal(SIGHUP, SIG_IGN);

set_signal_handler(SIGALRM);

set_signal_handler(SIGHUP);

I think SIGHUP should be ignored regardless? if it is captured we don't know if this will interrupt the library and break it? the signals will execute inside the libraries internal threads.

— Reply to this email directly

braindigitalis commented 4 months ago

There are many bots that don't use launchd etc for launching and will directly fork. these still need to be supported

⁣Kind regards

Craig Edwards

Brainbox.cc Creating fun stuff since 1993

On 6 Apr 2024, 22:27, at 22:27, Nidhoegger @.***> wrote:

Since not seeing this on GitHub directly, I will reply here and hope it works. IIRC SIGHUP is generated when the controlling TTY is lost without prior detaching (earlier it was called daemonization iirc). Today, frameworks like runit or launchd are used for daemonization of services, so if SIGHUP is generated, the controlling terminal is closed (most likely) and thus the process will die anyway. So IMHO it does not change anything if SIGHUP is ignored or not.

Am Sa., 6. Apr. 2024 um 18:12 Uhr schrieb Craig Edwards (Brain) < @.***>:

@.**** commented on this pull request.

In src/dpp/sslclient.cpp

https://github.com/brainboxdotcc/DPP/pull/1123#discussion_r1554634258:

@@ -237,11 +249,11 @@ ssl_client::ssl_client(const std::string &_hostname, const std::string &_port, b keepalive(reuse) {

ifndef WIN32

signal(SIGALRM, SIG_IGN);

signal(SIGHUP, SIG_IGN);

set_signal_handler(SIGALRM);

set_signal_handler(SIGHUP);

I think SIGHUP should be ignored regardless? if it is captured we don't know if this will interrupt the library and break it? the signals will execute inside the libraries internal threads.

— Reply to this email directly, view it on GitHub

https://github.com/brainboxdotcc/DPP/pull/1123#pullrequestreview-1984580864, or unsubscribe

https://github.com/notifications/unsubscribe-auth/A4EIMOO6QUJWGC3O7BNE44DY4ANG5AVCNFSM6AAAAABF2OPLL6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOBUGU4DAOBWGQ . You are receiving this because you authored the thread.Message ID: @.***>

-- Reply to this email directly or view it on GitHub: https://github.com/brainboxdotcc/DPP/pull/1123#issuecomment-2041209438 You are receiving this because you commented.

Message ID: @.***>

Nidhoegger commented 4 months ago

As said: If they have a controlling TTY and its closed, they will terminate, no matter if they ignore SIGHUP or not. If they fork and use setsid and detach from a terminal, then they wont get SIGHUP on closure. Ignoring SIGHUP will only make them not get the signal and terminate anyways.

Nidhoegger commented 4 months ago

Anyway, changed it. There is no harm done in ignoring it IMHO

Nidhoegger commented 4 months ago

To set everything to a known state. Not everything is checked and other fields might be set.Message ID: @.***>

braindigitalis commented 4 months ago

To set everything to a known state. Not everything is checked and other fields might be set.Message ID: @.***> …

surely the nicer c++ way is just: sa = {};

Nidhoegger commented 4 months ago

Feel free to change it. I dont see a reason to.

Jaskowicz1 commented 4 months ago

Is this latest change tested?

brainboxdotcc / DPP