On Thu, 27 Mar 2025 22:27:05 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 > > Larry Cable has updated the pull request incrementally with two additional > commits since the last revision: > > - Merge branch 'JDK-8344671' of github.com:larry-cable/jdk into JDK-8344671 > - JDK-8344671: removed JFR tests from problemlist resolved by this fix Looks good. Just a couple of remaining nits. Thanks. src/jdk.attach/macosx/native/libattach/VirtualMachineImpl.c line 148: > 146: > 147: if (caught && !ignored) { > 148: if (kill((pid_t)pid, SIGQUIT)) { Style nit: no implicit booleans Suggestion: if (kill((pid_t)pid, SIGQUIT) != 0) { ------------- Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/24085#pullrequestreview-2724337304 PR Review Comment: https://git.openjdk.org/jdk/pull/24085#discussion_r2017961485
