Skip to content

Commit

Permalink
[#420] Do not treat case of signal forwarded from MAIN worker thread …
Browse files Browse the repository at this point in the history
…as a new signal; Reuse pre-existing info/context from original signal handler for this case too

Fixes an occasional dual_fail_extend/dual_fail2_mustop_sigquit subtest failure where
a KILLBYSIGUINFO message is expected when another process sends a SIG-3 but instead we
see a KILLBYSIGSINFO1 message in the .mje file.

Below is one such stack trace where such an incorrect message gets sent out. While we were
handling the SIG-3 in a deferred fashion (through deferred_signal_handler()), another SIG-3
came in from the MAIN worker thread which drove generic_signal_handler() in a nested fashion
and caused the issue.

(gdb) where
 #0  __pthread_kill () at ../sysdeps/unix/sysv/linux/pthread_kill.c:57
 #1  gtm_dump_core () at sr_unix/gtm_dump_core.c:72
 #2  ch_cond_core () at sr_unix/ch_cond_core.c:77
 #3  rts_error_va () at sr_unix/rts_error.c:194
 #4  rts_error_csa () at sr_unix/rts_error.c:101
 #5  generic_signal_handler () at sr_unix/generic_signal_handler.c:195
 #6  <signal handler called>
 #7  semop () at ../sysdeps/unix/sysv/linux/semop.c:30
 #8  try_semop_get_c_stack () at sr_unix/gtm_c_stack_trace_semop.c:59
 #9  ftok_sem_lock () at sr_unix/ftok_sems.c:232
 #10 gds_rundown () at sr_unix/gds_rundown.c:324
 #11 gv_rundown () at sr_port/gv_rundown.c:123
 #12 gtm_exit_handler () at sr_unix/gtm_exit_handler.c:215
 #13 __run_exit_handlers () at exit.c:108
 #14 __GI_exit () at exit.c:139
 #15 gtm_image_exit () at sr_unix/gtm_image_exit.c:27
 #16 generic_signal_handler () at sr_unix/generic_signal_handler.c:361
 #17 ydb_stm_invoke_deferred_signal_handler () at sr_unix/ydb_stm_invoke_deferred_signal_handler.c:51
 #18 deferred_signal_handler () at sr_port/deferred_signal_handler.c:55
 #19 tp_tend () at sr_port/tp_tend.c:1887
 #20 op_tcommit () at sr_port/op_tcommit.c:496

This is now fixed by checking if the signal came in from another thread in the same process
(SI_TKILL) and if so treat this as a forwarded signal and not reset info/context but instead
reuse whatever was there from the original signal handler invocation.

A consequence of this change is that a pre-existing assert (that checked "stapi_signal_handler_deferred")
could now fail. That is now removed.
  • Loading branch information
nars1 committed Mar 25, 2019
1 parent c5c927b commit e46f313
Showing 1 changed file with 12 additions and 2 deletions.
14 changes: 12 additions & 2 deletions sr_unix/sig_init.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ GBLREF sig_info_context_t stapi_signal_handler_oscontext[sig_hndlr_num_entries];

#define STAPI_CLEAR_SIGNAL_HANDLER_DEFERRED(SIGHNDLRTYPE) \
{ \
assert(stapi_signal_handler_deferred & (1 << SIGHNDLRTYPE)); \
stapi_signal_handler_deferred &= ~(1 << SIGHNDLRTYPE); \
SHM_WRITE_MEMORY_BARRIER; \
stapi_signal_handler_oscontext[SIGHNDLRTYPE].sig_forwarded = FALSE; \
Expand Down Expand Up @@ -195,7 +194,18 @@ GBLREF sig_info_context_t stapi_signal_handler_oscontext[sig_hndlr_num_entries];
int i, tLevel; \
boolean_t signalForwarded; \
\
if (DUMMY_SIG_NUM != SIG) \
/* If SIG is DUMMY_SIG_NUM, we are guaranteed this is a forwarded signal. Otherwise, it could still be forwarded \
* from a "pthread_kill" invocation from the MAIN worker thread or the thread that randomly got the original signal. \
* We find that out by using the linux-only SI_TKILL info code to check if this signal came from a "tkill" system call. \
* Even if the info code is SI_TKILL, it is possible a completely different signal than what was recorded before comes \
* in. In that case, do not treat that as a forwarded signal but as a new signal. Hence the "sig_num" check below. \
*/ \
assert((DUMMY_SIG_NUM == SIG) || (NULL != INFO)); \
signalForwarded = ((DUMMY_SIG_NUM == SIG) \
|| (simpleThreadAPI_active \
&& (SI_TKILL == INFO->si_code) \
&& (SIG == stapi_signal_handler_oscontext[SIGHNDLRTYPE].sig_num))); \
if (!signalForwarded) \
{ /* This is not a forwarded signal */ \
if (simpleThreadAPI_active) \
{ /* \
Expand Down

0 comments on commit e46f313

Please sign in to comment.