fix: expand ReentrantLock scope in MonitorThreadContainer for better …

[aaronchung-bitquill]

…thread safety

Summary

Remove race condition in MonitorThreadContainer between getInstance() and releaseInstance() methods.

Description

In the MonitorThreadContainer, there are possible race conditions between the getInstance() and releaseInstance() methods.

In one of the race conditions is in the getInstance().

...
      } finally {
        LOCK_OBJECT.unlock();
      }
    }
    CLASS_USAGE_COUNT.getAndIncrement();
    return singleton;
  }

Just after the lock object is unlocked, is an opportunity for a race condition with another thread in the releaseInstance() method.

...
    if (CLASS_USAGE_COUNT.decrementAndGet() <= 0) {
      LOCK_OBJECT.lock();
      try {
        if (singleton != null) {
          singleton.releaseResources();
          singleton = null;
          CLASS_USAGE_COUNT.set(0);
...

If the locked block is entered, a race condition may result. getInstance() may return a null singleton which will be followed by a NPE. Or another scenario is that a non-null singleton will be returned, but singleton.releaseResources() is called so the MonitorThreadContainer's threadPool will be shutdown and subsequent usage of the threadPool will result in a RejectedExecutionException.

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[aaronchung-bitquill]

Just realized that perhaps using synchronized on both methods may make more sense since the scope of the locks are pretty much the entirety of both getInstance() and releaseInstance().

[brunos-bq]

NIT: can we rename it to ConcurrencyEFMTest ?

[aaronchung-bitquill]

ConcurrencyEFMTest may be too general. There is also another pre-existing test class MultiThreadedDefaultMonitorServiceTest that also tests concurrency in EFM.

[aaronchung-bitquill]

Is the nit b/c MultiThreadedMonitorThreadContainerTest is extremely verbose? :D

I was thinking of replacing MultiThreaded with Concurrency so the test class name would be ConcurrencyMonitorTheadContainerTest but that still seems a bit wordy.

What do you think?

[davecramer]

Instead of using synchronized it would be better to use the re-entrant lock before the first check. synchronized is still a heavier lock.

[karenc-bq]

Suggested change

	@Mock
	ExecutorServiceInitializer mockExecutorServiceInitializer;
	@Mock ExecutorServiceInitializer mockExecutorServiceInitializer;

[karenc-bq]

Suggested change

	@Mock
	ExecutorService mockExecutorService;
	@Mock ExecutorService mockExecutorService;