[19587] Include variety of terminate proccess signals handler in discovery server #4278

JesusPoderoso · 2024-01-23T15:53:55Z

Description

While closing the terminal by clicking the X button, it seems that the signal emitted was SIGHUP.
For that reason, by handling that signal the application properly closes in those scenarios.

It may be interesting to handle this signal also in several eProsima products. What do you think about the following list, @Mario-DL?
Fast DDS CLI, Fast DDS pipe, router, monitor, record, replay, spy.

EDIT:
After research , I've included the following signals to handle as terminate:

SIGQUIT: Quit signal. Similar to SIGINT, but also produces a core dump of the process.

@Mergifyio backport 2.12.x 2.11.x 2.10.x 2.6.x

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added; the added tests pass locally
N/A Any new/modified methods have been properly documented using Doxygen.
Changes are ABI compatible.
Changes are API compatible.
N/A New feature has been added to the versions.md file (if applicable).
N/A New feature has been documented/Current behavior is correctly described in the documentation.
Applicable backports have been included in the description.

Reviewer Checklist

The PR has a milestone assigned.
The title and description correctly express the PR's purpose.
Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

Signed-off-by: JesusPoderoso <[email protected]>

Mario-DL · 2024-01-25T10:16:21Z

@richiprosima please test this

Mario-DL

Interesting signal additions. LGTM with Green CI.
The list is Ok to me. SIGKILL and SIGSTOP cannot be handled.
Checked that it solves the issue of an unhandled signal

Mario-DL · 2024-01-25T10:19:52Z

Friendly pinging @rsanchez15 as it could be interesting to handle these signals in other executables or products too.

Mario-DL · 2024-01-25T10:21:15Z

@JesusPoderoso NIT
Please, update the PR's title since SIGHUP is not going to be the only signal handled in this PR.

Signed-off-by: JesusPoderoso <[email protected]>

Mario-DL

LGTM with Green CI. Checking that the CI is compiling its important since some signals may not be available in all OSs.

Mario-DL · 2024-01-25T10:28:09Z

@richiprosima please test this

tools/fds/server.cpp

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso · 2024-01-25T10:40:09Z

@richiprosima please test this

JesusPoderoso · 2024-01-26T07:07:01Z

@richiprosima please test windows

Mario-DL · 2024-01-26T07:11:24Z

@JesusPoderoso please include the backports in the description

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso · 2024-01-26T12:56:26Z

It seems that SIGQUIT is also NOT supported in Windows @Mario-DL @MiguelCompany

JesusPoderoso · 2024-01-26T12:56:57Z

@Mergifyio backport 2.12.x 2.11.x 2.10.x 2.6.x

mergify · 2024-01-26T12:57:08Z

backport 2.12.x 2.11.x 2.10.x 2.6.x

✅ Backports have been created

#4331 [19587] Include variety of terminate proccess signals handler in discovery server (backport #4278) has been created for branch 2.12.x
#4332 [19587] Include variety of terminate proccess signals handler in discovery server (backport #4278) has been created for branch 2.11.x
#4333 [19587] Include variety of terminate proccess signals handler in discovery server (backport #4278) has been created for branch 2.10.x
#4334 [19587] Include variety of terminate proccess signals handler in discovery server (backport #4278) has been created for branch 2.6.x

JesusPoderoso · 2024-01-26T12:57:10Z

@richiprosima please test this

JesusPoderoso · 2024-01-29T07:00:00Z

@richiprosima please test mac

JesusPoderoso · 2024-01-29T12:05:12Z

@richiprosima please test mac

JesusPoderoso · 2024-01-30T06:48:12Z

@richiprosima please test mac

JesusPoderoso · 2024-01-30T14:25:38Z

@richiprosima please test mac

JesusPoderoso · 2024-01-30T14:36:36Z

@richiprosima please test mac

JesusPoderoso · 2024-01-30T14:43:26Z

@richiprosima please test mac 🥲

JesusPoderoso · 2024-01-30T14:47:53Z

@richiprosima please test mac 😢

…rver (#4278) * Refs #19587: Include SIGHUP handler Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Handle more signals Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Move proper signals to 'linux only' case Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Apply Miguel suggestion Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Fix Windows build Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 6eb1170)

…ver (#4278) (#4332) * Refs #19587: Include SIGHUP handler Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Handle more signals Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Move proper signals to 'linux only' case Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Apply Miguel suggestion Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Fix Windows build Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 6eb1170) Co-authored-by: Jesús Poderoso <[email protected]>

…rver (#4278) (#4333) * Refs #19587: Include SIGHUP handler Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Handle more signals Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Move proper signals to 'linux only' case Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Apply Miguel suggestion Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Fix Windows build Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 6eb1170) Co-authored-by: Jesús Poderoso <[email protected]>

…rver (#4278) (#4331) * Refs #19587: Include SIGHUP handler Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Handle more signals Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Move proper signals to 'linux only' case Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Apply Miguel suggestion Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Fix Windows build Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 6eb1170) Co-authored-by: Jesús Poderoso <[email protected]>

…rver (#4278) (#4334) * Refs #19587: Include SIGHUP handler Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Handle more signals Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Move proper signals to 'linux only' case Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Apply Miguel suggestion Signed-off-by: JesusPoderoso <[email protected]> * Refs #19587: Fix Windows build Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: JesusPoderoso <[email protected]> (cherry picked from commit 6eb1170) Co-authored-by: Jesús Poderoso <[email protected]>

Refs #19587: Include SIGHUP handler

f464195

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso added the no-test Skip CI tests if PR marked with this label label Jan 23, 2024

JesusPoderoso added this to the v2.13.2 milestone Jan 23, 2024

Refs #19587: Handle more signals

0a4310a

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso force-pushed the hotfix/19587 branch from e95b28c to 0a4310a Compare January 24, 2024 22:44

Mario-DL previously approved these changes Jan 25, 2024

View reviewed changes

Refs #19587: Move proper signals to 'linux only' case

c4930c0

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso dismissed Mario-DL’s stale review via c4930c0 January 25, 2024 10:22

JesusPoderoso changed the title ~~[19587] Include SIGHUP handler in discovery server~~ [19587] Include variety of terminate proccess signals handler in discovery server Jan 25, 2024

Mario-DL previously approved these changes Jan 25, 2024

View reviewed changes

MiguelCompany reviewed Jan 25, 2024

View reviewed changes

tools/fds/server.cpp Outdated Show resolved Hide resolved

Refs #19587: Apply Miguel suggestion

bc4a6f4

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso dismissed Mario-DL’s stale review via bc4a6f4 January 25, 2024 10:39

MiguelCompany previously approved these changes Jan 25, 2024

View reviewed changes

Refs #19587: Fix Windows build

91792ae

Signed-off-by: JesusPoderoso <[email protected]>

JesusPoderoso dismissed MiguelCompany’s stale review via 91792ae January 26, 2024 12:56

JesusPoderoso added the ci-pending PR which CI is running label Jan 29, 2024

JesusPoderoso added ready-to-merge Ready to be merged. CI and changes have been reviewed and approved. and removed no-test Skip CI tests if PR marked with this label ci-pending PR which CI is running labels Jan 30, 2024

MiguelCompany approved these changes Jan 30, 2024

View reviewed changes

JesusPoderoso merged commit 6eb1170 into master Jan 30, 2024
14 checks passed

JesusPoderoso deleted the hotfix/19587 branch January 30, 2024 14:59

mergify bot mentioned this pull request Jan 30, 2024

[19587] Include variety of terminate proccess signals handler in discovery server (backport #4278) #4334

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[19587] Include variety of terminate proccess signals handler in discovery server #4278

[19587] Include variety of terminate proccess signals handler in discovery server #4278

JesusPoderoso commented Jan 23, 2024 •

edited by MiguelCompany

Loading

Mario-DL commented Jan 25, 2024

Mario-DL left a comment •

edited

Loading

Mario-DL commented Jan 25, 2024

Mario-DL commented Jan 25, 2024

Mario-DL left a comment

Mario-DL commented Jan 25, 2024

JesusPoderoso commented Jan 25, 2024

JesusPoderoso commented Jan 26, 2024

Mario-DL commented Jan 26, 2024

JesusPoderoso commented Jan 26, 2024

JesusPoderoso commented Jan 26, 2024

mergify bot commented Jan 26, 2024 •

edited

Loading

JesusPoderoso commented Jan 26, 2024

JesusPoderoso commented Jan 29, 2024

JesusPoderoso commented Jan 29, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

[19587] Include variety of terminate proccess signals handler in discovery server #4278

[19587] Include variety of terminate proccess signals handler in discovery server #4278

Conversation

JesusPoderoso commented Jan 23, 2024 • edited by MiguelCompany Loading

Description

Contributor Checklist

Reviewer Checklist

Mario-DL commented Jan 25, 2024

Mario-DL left a comment • edited Loading

Choose a reason for hiding this comment

Mario-DL commented Jan 25, 2024

Mario-DL commented Jan 25, 2024

Mario-DL left a comment

Choose a reason for hiding this comment

Mario-DL commented Jan 25, 2024

JesusPoderoso commented Jan 25, 2024

JesusPoderoso commented Jan 26, 2024

Mario-DL commented Jan 26, 2024

JesusPoderoso commented Jan 26, 2024

JesusPoderoso commented Jan 26, 2024

mergify bot commented Jan 26, 2024 • edited Loading

✅ Backports have been created

JesusPoderoso commented Jan 26, 2024

JesusPoderoso commented Jan 29, 2024

JesusPoderoso commented Jan 29, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 30, 2024

JesusPoderoso commented Jan 23, 2024 •

edited by MiguelCompany

Loading

Mario-DL left a comment •

edited

Loading

mergify bot commented Jan 26, 2024 •

edited

Loading