On Thu, 11 May 2023 21:22:33 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> In TestScaffold.java we check the debuggee process exitValue, and allow a 0 >> or a 1. Otherwise the test fails. You get exitValue 1 when the debuggee >> exits with an exception. Allowing this was necessary because some tests >> purposely make the debuggee exit with an exception. However, this runs the >> risk of not detecting a debuggee exception when none was expected. >> >> [JDK-8306758](https://bugs.openjdk.org/browse/JDK-8306758) added support for >> allowing the test to determine which exitValues are acceptable. This means >> we can now change TestScaffold to by default only allow exitValue 0. Tests >> that expect an exception can override `TestScaffold.allowedExitValue(int)` >> to allow (or only expect) an exitValue of 1. There are 2 test that will need >> updating once TestScaffold by default no longer allows exitValue 1: >> >> - ResumeOneThread.java has a latent virtual thread bug that this PR is >> exposing. In order to fix >> [JDK-8283796](https://bugs.openjdk.org/browse/JDK-8283796), the debuggee is >> doing a Thread.setDaemon(false) on the threads it creates. This results in >> an exception when used on a virtual thread, causing the debuggee to exit >> with exitValue 1, which previously went undetected. The changes for the CR >> no longer allow exitValue 1, exposing this bug. The proper fix for >> [JDK-8283796](https://bugs.openjdk.org/browse/JDK-8283796) should have been >> to instead Thread.join() on the created threads. >> >> - ExceptionEvents.java has some test modes that expect the debuggee to exit >> with an exception, so `allowedExitValue()` is overridden to expect exitValue >> 1 instead of 0 for these test modes. >> >> There is also one minor fix needed in TestScaffold when using virtual >> threads. The virtual thread factory related code includes a catch clause to >> catch all exceptions. It does this in the code that invokes the virtual >> thread main method. If any exception is caught, it is saved away. I think >> the intent was to rethrow it in the main thread just like it would have been >> thrown if not using the virtual thread factory, allowing the debuggee to >> exit with the exception (and exitValue 1). However, the code wasn't >> rethrowing it, so if an exception was thrown by the debuggee virtual thread, >> the test still exited with exitValue 0. This causes unexpected debuggee >> exceptions to go unnoticed, and also causes tests that expect a debuggee >> exception to fail. I tried to fix this by rethrowing the exception, but this >> causes some tests that track ExceptionEvents to complain about an unexpected >> excep... > > Chris Plummer has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'master' into 8307559_exitValue > merge with master branch > - fix jcheck error > - Don't allow exitValue 1 by default. Thanks for the reviews Kevin and Leonid! ------------- PR Comment: https://git.openjdk.org/jdk/pull/13919#issuecomment-1544814142
