On Mon, 17 Mar 2025 18:26:57 GMT, Larry Cable <d...@openjdk.org> wrote:
> on both Linux and MacOS libattach utilizes UNIX signal (QUIT) to cause a > target JVM (attachee) to create the socket file used as transport for > subsequent jcmds (and other attach based interactions) and to listen upon > that for such. > > it should be noted that the default behavior for QUIT (if not blocked or > caught) is to terminate the signalled process. > > during the early lifetime of a JVM, its signal handlers are not yet > installed, and thus any signal such as QUIT will cause the > default behavior to occur, in this case the JVM will be terminated. > > this is why some tests are failing with "not alive" > > the "fix" is similar in nature to that already implemented for linux (however > using a different OS dependent mechanism to obtain the attachee JVM's signal > masks: sysctl(2)). > > the method "checkCatchesAndSendQuitTo" will now obtain the "attachee" JVM > signal masks and only kill(QUIT) if the > current masks indicate that the JVM's signals are now being handled. > > the behavior in the success case is now identical to the previous > implementation, however should the target JVM not > become "ready" (signal handlers installed) prior to the attach "timeout" > occurring the attach operation will throw an > "AttachNotSupportedException" with a suitable error message. > > see also: https://bugs.openjdk.org/browse/JDK-8350766 Thanks very much for fixing this @larry-cable ! Changes look good. Just a few minor nits. Also you need to remove the ProblemList entries in test/jdk/ProblemList.txt. src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 126: > 124: > 125: /* > 126: * early in the lifetime of a JVM it has not yet initialized its > signal handlers, in particular the QUIT Suggestion: * Early in the lifetime of a JVM it has not yet initialized its signal handlers, in particular the QUIT src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 129: > 127: * handler, note that the default behavior of QUIT is to terminate > the receiving process, if unhandled. > 128: * > 129: * since we use QUIT to initiate an attach operation, if we signal a > JVM during this period early in its Suggestion: * Since we use QUIT to initiate an attach operation, if we signal a JVM during this period early in its src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 133: > 131: * are attempting to attach to! > 132: * > 133: * the following code guards the QUIT delivery by testing the current > signal masks Suggestion: * The following code guards the QUIT delivery by testing the current signal masks. It is okay to send QUIT * if the signal is caught but not ignored, as that implies a handler has been installed. src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 138: > 136: if (sysctl(mib, sizeof(mib) / sizeof(int), &kiproc, &kipsz, NULL, 0) > == 0) { > 137: const int ignored = (kiproc.kp_proc.p_sigignore & > sigmask(SIGQUIT)) != 0; > 138: const int caught = (kiproc.kp_proc.p_sigcatch & > sigmask(SIGQUIT)) != 0; Suggestion: const bool ignored = (kiproc.kp_proc.p_sigignore & sigmask(SIGQUIT)) != 0; const bool caught = (kiproc.kp_proc.p_sigcatch & sigmask(SIGQUIT)) != 0; src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 140: > 138: const int caught = (kiproc.kp_proc.p_sigcatch & > sigmask(SIGQUIT)) != 0; > 139: > 140: // *only* send QUIT if the target is ready to catch and handle > the signal to avoid default "death" if not Delete this comment. src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 143: > 141: > 142: // note: obviously the masks could change between testing and > signalling however this is not the > 143: // observed behavior of the current JVM implementation. The mask should only change in one direction - from default behaviour to being handled - so this "race" is not a concern. src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 146: > 144: > 145: if (caught && !ignored) { > 146: if (kill((pid_t)pid, SIGQUIT)) { ```suggestion (no implicit booleans) if (kill((pid_t)pid, SIGQUIT) != 0) { src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 154: > 152: char msg[100]; > 153: > 154: snprintf(msg, sizeof(msg), "%d: state is not ready to participate > in attach handshake!", (int)pid); Suggestion: snprintf(msg, sizeof(msg), "%d: process state is not ready to participate in attach handshake!", (int)pid); Without the `cmd` the original message didn't quite read right. ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24085#pullrequestreview-2692365572 PR Comment: https://git.openjdk.org/jdk/pull/24085#issuecomment-2731246857 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999848252 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999849256 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999849992 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999874844 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999865604 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999864741 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999868536 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r1999869609
