HDDS-6658. BackgroundPipelineCreator does not always stop quickly

[sodonnel]

What changes were proposed in this pull request?

On my laptop, TestSCMSafeModeManager.testSafeModePipelineExitRule() always takes just over 200 seconds. Checking a few PR runs, it does not seem to be slow on PRs, just locally.

Debugging the code, it seems to hang in the BackgroundPipelineCreator.stop() method, where it is waiting for the tread to join:

  public void stop() {
    if (!running.compareAndSet(true, false)) {
      LOG.warn("{} is not running, just ignore.", THREAD_NAME);
      return;
    }

    LOG.info("Stopping {}.", THREAD_NAME);

    // in case RatisPipelineUtilsThread is sleeping
    synchronized (monitor) {
      monitor.notifyAll();
    }

    try {
      thread.join();  //  ----> Hangs here
    } catch (InterruptedException e) {
      LOG.warn("Interrupted during join {}.", THREAD_NAME);
      Thread.currentThread().interrupt();
    }
  }

It is clearly hanging as the background thread did not exit, and I believe it is because notify() is being used to try to exit the thread, when it should really be interrupted. There is a chance that notify is called while the thread is not waiting, and if so, it will just fall back into the wait state and not exit until it wakes up again.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6658

How was this patch tested?

Validated slow test is now fast locally.

[sodonnel]

Unit test failure:

Failures: 
Error:    TestBackgroundPipelineScrubber.testRun:97 

This is #3359 / #3359

[szetszwo]

@sodonnel , thanks for working on this. The change looks good. Just two minor comments inlined.

[szetszwo]

We should remove "synchronized" since interrupt() does not require it.

[szetszwo]

I suggest to keep the private monitor field. It can avoid code outside this class calling wait/notify/notifyAll.

[sodonnel]

@szetszwo Thanks for the review. I have rolled back the earlier changes and implemented again in a new commit. Please have another look when you get a chance.

[szetszwo]

+1 the change looks good.