RFR(s): 8179327: gtestLauncher should run tests on a separate thread (optionally)

[Mikael Gerdin]

Hi Thomas,

On 2017-07-17 16:33, Thomas Stüfe wrote:
> Hi Mikael,
> 
> thanks for your review!
> 
> New Webrev: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8179327-gtestLauncher-should-have-an-option-to-avoid-primordial-thread-for-tests/webrev.01/webrev/
> 
> See comments inline.

Thanks for fixing this, it looks a lot nicer now.
I can sponsor the change for you. I think Robbin is on vacation but I 
don't think he'll mind a bit of additional cleanups.

/Mikael

> 
> On Mon, Jul 17, 2017 at 10:56 AM, Mikael Gerdin 
> <mikael.gerdin at oracle.com <mailto:mikael.gerdin at oracle.com>> wrote:
> 
>     Hi Thomas,
> 
>     On 2017-07-13 08:18, Thomas Stüfe wrote:
> 
>         Ping..
> 
>         May I please get a second review and a sponsor?
> 
> 
>     I would prefer if you made a
>     const static bool DEFAULT_SPAWN_IN_NEW_THREAD which is initialized
>     with the proper platform value. The current 5-line return statement
>     in get_spawn_new_main_thread_arg looks a bit weird.
> 
> 
> Good suggestion, fixed.
> 
> 
>     If you move the wrapping of argc/argv up to runUnitTests and pass
>     the args_t instead you can make run_in_new_thread completely
>     platform specific instead of having #ifdef _WIN32 in the middle, I
>     think this is the preferred style in the code base overall.
> 
> 
> Okay. I did this and shuffled a bit code around, so now we only have one 
> #ifdef _WIN32 section, should be better readable now.
> 
>     In the pthread join error path you have
>     "/rsyn" at the end of the line, is that intentional?
> 
> 
> No, good catch, that was a stray console command.
> 
> Kind Regards, Thomas
> 
> 
>     Thanks
>     /Mikael
> 
> 
> 
>         Thanks, Thomas
> 
>         On Tue, Jul 4, 2017 at 11:12 AM, Thomas Stüfe
>         <thomas.stuefe at gmail.com <mailto:thomas.stuefe at gmail.com>>
>         wrote:
> 
> 
> 
>             On Tue, Jul 4, 2017 at 10:22 AM, Robbin Ehn
>             <robbin.ehn at oracle.com <mailto:robbin.ehn at oracle.com>> wrote:
> 
>                 Looks reasonable, thanks!
> 
> 
>             Thank you!
> 
> 
>                 /Robbin
> 
>                 If you don't mind, please don't wrap lines, links do not
>                 work well with
>                 line breaks in them :)
> 
> 
>             Oops, sorry!
> 
> 
>                 On 07/02/2017 09:23 AM, Thomas Stüfe wrote:
> 
>                     Dear all,
> 
>                     please review the following change:
> 
>                     Bug: JDK-8179327
>                     <https://bugs.openjdk.java.net/browse/JDK-8179327
>                     <https://bugs.openjdk.java.net/browse/JDK-8179327>>
>                     Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/
>                     <http://cr.openjdk.java.net/~stuefe/webrevs/>
>                     8179327-gtestLauncher-should-have-an-option-to-avoid-
>                     primordial-thread-for-tests/webrev.00/webrev/
> 
>                     This fix is motivated by the inability of the AIX
>                     openjdk vm to run on
>                     the
>                     primordial thread (see
>                     https://bugs.openjdk.java.net/browse/JDK-8171505
>                     <https://bugs.openjdk.java.net/browse/JDK-8171505>
>                     ),
>                     which we unfortunately cannot do much about. This
>                     prevented us until now
>                     to
>                     run gtests successfully, because the gtestlauncher
>                     (in contrast to the
>                     standard java launcher) invokes the VM on the
>                     primordial thread.
> 
>                     To fix this, I gave the gtest launcher the ability
>                     to run the tests in an
>                     own thread. Instead of sprinkling the source with
>                     "#ifdef _AIX", I added
>                     a
>                     command line option "-new-thread", which allows to
>                     start the tests in a
>                     new
>                     thread, and implemented this feature for all
>                     platforms (well, posix and
>                     windows). On all platforms save AIX this is by
>                     default off, so nothing
>                     changes. On AIX, tests will be default be executed
>                     in a new thread.
> 
>                     Thanks for reviewing,
> 
>                     Kind Regards, Thomas
> 
> 
> 
>