On Thu, 30 Mar 2023 21:48:22 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
>> line 129:
>>
>>> 127: }
>>> 128:
>>> 129: static synchronized private void startThreads() {
>>
>> So making this method synchronized instead of startThread() will make it
>> less likely that we will face the previous issue, but it is still timing
>> dependent, because the call to start the launcher can return before the
>> launcher reaches here. It will be very unlikely given the sleeps but if we
>> want to guard against any surprises we could have a variable set in
>> startThreads() and in finishThreads() we check and wait until that variable
>> is set.
>
> Thank you for the concern.
> The `startThread()` below which is called from `startThreads()` has the call
> to `thread.ensureReady()` which waits until the target tested thread really
> starts and sets the `threadReady` field. So, there is no race condition as
> the `startThreads()` and `finishThreads()` are synchronized methods.
>
>
> static private void startThread(int i) {
> String name = "TestedThread" + i;
> TestedThread thread = new TestedThread(name);
> vts[i] = Thread.ofVirtual().name(name).start(thread);
> thread.ensureReady();
> threads[i] = thread;
> log("# Java: started vthread: " + name);
> }
>
> static synchronized private void startThreads() {
> log("\n# Java: Starting vthreads");
> for (int i = 0; i < VTHREADS_CNT; i++) {
> sleep(1);
> startThread(i);
> }
> }
>
> static private synchronized void finishThreads() {
> try {
> for (int i = 0; i < VTHREADS_CNT; i++) {
> TestedThread thread = threads[i];
> thread.letFinish();
> vts[i].join();
> }
> } catch (InterruptedException e) {
> throw new RuntimeException(e);
> }
> }
>
> Please, let me know if I'm missing anything.
So the race I am talking about is between the main thread running
finishThreads() and the launcher thread running startThreads(). The main thread
could execute finishThreads() before the launcher executes startThreads(). If
you comment out the two first sleeps in run_test_cycle() you can actually see
the issue. Again, given that the sleeps are there it is an unlikely scheduling,
but if we want to avoid depending on timing we can add that extra
synchronization.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1153872917
