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

src: only block SIGUSR1 when HAVE_INSPECTOR #12266

Closed
wants to merge 1 commit into from

Conversation

@danbev
Copy link
Member

commented Apr 7, 2017

I'm currently seeing a timeout error for test-signal-handler.js on
macosx when using the following configuration:

./configure --debug --without-ssl && make -j8 test

--without-ssl implies that there will be no inspector but the signal
SIGUSR1 is blocked in PlatformInit just the same. But in this case
never unblocked which is causing the signal to never be delivered to
the handlers in test-signal-handler.js and it loops until it times out.

Not sure if this is the best way of fixing this but hopefully more eyes
on this will help.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

@bnoordhuis
Copy link
Member

left a comment

One more for nodejs/build#419.

src/node.cc Outdated
sigset_t sigmask;
sigemptyset(&sigmask);
sigaddset(&sigmask, SIGUSR1);
const int err = pthread_sigmask(SIG_SETMASK, &sigmask, nullptr);
CHECK_EQ(err, 0);
#endif

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 7, 2017

Member

Can you write this as #endif // HAVE_INSPECTOR? (Note the two blanks before the comment.)

You can't really do the CHECK_EQ at this point because fds 0-2 might not be pointing to valid file descriptions yet.

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

Can you write this as #endif // HAVE_INSPECTOR? (Note the two blanks before the comment.)

Yep, I'll fix that.

You can't really do the CHECK_EQ at this point because fds 0-2 might not be pointing to valid file descriptions yet.

Sorry I'm missing something here :( CHECK_EQ is checking the return value from pthread_sigmask and err is const. How do the file descriptors effect this?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 7, 2017

Check the for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) loop right below it.

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

Check the for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) loop right below it.

Oh! Let's not speak of this again :)

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2017

src/node.cc Outdated
CHECK_EQ(err, 0);
#endif // HAVE_INSPECTOR

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 7, 2017

Member

I don't want to make this a torment for you but the reason SIGUSR1 is blocked first thing is to make the 'kill window' as small as possible. Some tools start node and immediately send a SIGUSR1 - if the signal isn't masked, it kills the process.

This comment has been minimized.

Copy link
@danbev

danbev Apr 7, 2017

Author Member

Ah I see, that makes sense. I was trying to avoid the additional HAVE_INSTRUCTOR guard but that is no problem.

I don't want to make this a torment

No torment at all, I appreciate all the reviews and feedback!

src: only block SIGUSR1 when HAVE_INSPECTOR
I'm currently seeing a timeout error for test-signal-handler.js on
macosx when using the following configuration:

./configure --debug --without-ssl && make -j8 test

--without-ssl implies that there will be no inspector but the signal
SIGUSR1 is blocked in PlatformInit just the same. But in this case
never unblocked which is causing the signal to never be delivered to
the handlers in test-signal-handler.js and it loops until it times out.

Not sure if this is the best way of fixing this but hopefully more eyes
on this will help.

@danbev danbev force-pushed the danbev:test-signal-handler-failure branch to d9d1164 Apr 9, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2017

danbev added a commit to danbev/node that referenced this pull request Apr 10, 2017
src: only block SIGUSR1 when HAVE_INSPECTOR
I'm currently seeing a timeout error for test-signal-handler.js on
macosx when using the following configuration:

./configure --debug --without-ssl && make -j8 test

--without-ssl implies that there will be no inspector but the signal
SIGUSR1 is blocked in PlatformInit just the same. But in this case
never unblocked which is causing the signal to never be delivered to
the handlers in test-signal-handler.js and it loops until it times out.

Not sure if this is the best way of fixing this but hopefully more eyes
on this will help.

PR-URL: nodejs#12266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

Landed in 65a6e05

@danbev danbev closed this Apr 10, 2017

@danbev danbev deleted the danbev:test-signal-handler-failure branch Apr 10, 2017

@italoacasas

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

cc @danbev

danbev added a commit to danbev/node that referenced this pull request Apr 11, 2017
src: only block SIGUSR1 when HAVE_INSPECTOR
I'm currently seeing a timeout error for test-signal-handler.js on
macosx when using the following configuration:

./configure --debug --without-ssl && make -j8 test

--without-ssl implies that there will be no inspector but the signal
SIGUSR1 is blocked in PlatformInit just the same. But in this case
never unblocked which is causing the signal to never be delivered to
the handlers in test-signal-handler.js and it loops until it times out.

Not sure if this is the best way of fixing this but hopefully more eyes
on this will help.

PR-URL: nodejs#12266
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@danbev

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2017

@italoacasas I've added #12332 for this and two other commits needed to land this on the v7.x-staging branch.

@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Could you retarget #12332 at v6.x-staging (or close it and open a new one)? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.