Skip to content

Ignore SIGURG at Start command #327

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

Merged
merged 2 commits into from
Feb 10, 2023
Merged

Conversation

vr009
Copy link
Contributor

@vr009 vr009 commented Feb 2, 2023

This patch fixes the problem, which was occurring when the watchdog
process received a signal SIGURG from go runtime[1][2] and passed it
to the forked process before the exec call. Fixed by adding a call of
the Ignore function. Receiving this signal is unexpected, cause tt
doesn't work with sockets at all.

Also this patch fixes the signals handling strategy. Now there is only
one handling loop for all signals. Added two fields for controlling
a state of the instance being watched and a mutex for synchronizing
the goroutines changing them.

[1] - https://go.googlesource.com/proposal/+/master/design/24543-non-cooperative-preemption.md
[2] - golang/go#37942

Closes #325

@vr009 vr009 requested a review from psergee February 2, 2023 13:50
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch from fcbf8cf to ae51d79 Compare February 2, 2023 13:58
@vr009 vr009 changed the title Ignore SIGINT at Start command Ignore SIGURG at Start command Feb 3, 2023
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 3 times, most recently from 105367b to 89d2ca1 Compare February 3, 2023 15:17
@vr009 vr009 marked this pull request as ready for review February 3, 2023 15:48
@vr009 vr009 requested a review from psergee February 3, 2023 16:40
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 9 times, most recently from 9803cd4 to fe6517a Compare February 6, 2023 15:48
@LeonidVas LeonidVas self-requested a review February 6, 2023 19:28
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch from fe6517a to ac386b1 Compare February 7, 2023 10:05
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thank you for the patchset.
It's the review of the first commit.

  • Add Part of #... to the commit message.
  • Strange that it was not necessary to make changes to the Watchdog tests (Are you sure you don't need them?)
  • I don't like the fact that you have to start the signal processing from the outside. Would it make sense to add a callback that would be set externally and called by watchdog after the start of the application or something like that?

@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 3 times, most recently from 76b583e to f11c03c Compare February 8, 2023 05:54
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 4 times, most recently from 3a542ad to 75ca3d3 Compare February 8, 2023 06:34
@vr009
Copy link
Contributor Author

vr009 commented Feb 8, 2023

Strange that it was not necessary to make changes to the Watchdog tests (Are you sure you don't need them?).

I don't like the fact that you have to start the signal processing from the outside. Would it make sense to add a callback that would be set externally and called by watchdog after the start of the application or something like that?

Check the idea, please.

@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch from 75ca3d3 to 40605b9 Compare February 8, 2023 06:47
@vr009 vr009 requested a review from LeonidVas February 8, 2023 06:49
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 2 times, most recently from 7c6e8fa to 078c8ff Compare February 8, 2023 07:14
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch 2 times, most recently from 96bdfae to ea4ad6d Compare February 8, 2023 11:28
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch from ea4ad6d to 04eaab3 Compare February 8, 2023 12:27
@vr009 vr009 added the full-ci Enables full ci tests label Feb 8, 2023
This patch fixes the signals handling strategy. Now there is only
one handling loop for all signals. Added a field for controlling
a state of the watchdog and a mutex for synchronizing the goroutines
changing this field.

Part of #325
This patch fixes the problem, which was occurring when the watchdog
process received a signal SIGURG from go runtime[1][2] and passed it
to the forked process before the exec call. Fixed by adding a call of
the Ignore function. Receiving this signal is unexpected, cause tt
doesn't work with sockets at all.

[1] - https://go.googlesource.com/proposal/+/master/design/24543-non-cooperative-preemption.md
[2] - golang/go#37942

Closes #325
@vr009 vr009 force-pushed the vr009/gh-325-fix-race-condition branch from 04eaab3 to 0b78673 Compare February 10, 2023 12:35
@LeonidVas LeonidVas merged commit aa82657 into master Feb 10, 2023
@LeonidVas LeonidVas deleted the vr009/gh-325-fix-race-condition branch February 10, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full-ci Enables full ci tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tt start/stop commands does not always work
3 participants