1

I'm coding an enum strategy pattern where one of the strategies makes use of an ScheduledExecutor:

class ControllerImpl { //... boolean applyStrat(StratParam param) { getStrat().apply(param); } //... private enum Strats { NOOP (NoopStrategyImpl.class), REAL (RealtimeStrategyImpl.class), SCHED (ScheduledStrategyImpl.class), ; private final Strategy strategy; Mode(Class<? extends Strategy> strategyClass) { this.strategy = getInjector().getInstance(strategyClass); } public boolean applyStrategy(StratParam param) { return this.strategy.apply(param); } } class ScheduledStrategyImpl implements Strategy { private ScheduledExecutor executor = Executors.new..().schedule..(); @Override public boolean apply(StratParam param) { // actual apply() logic } }

The goal of doing it like this, is to be able to hot-swap strategies. The default strategy is REAL, but during runtime it may be switched to NOOP or SCHED.

Now, the above code initializes the executor during static initialization of the enum. This means that SCHED will contain a running executor -- even if it is never used.
But I don't want to start the executor unless the strategy is actually used (no point in having it running in the background for no reason).
The common solution for this is to lazily initialize the executor in the apply() method instead:

class ExecutorStrategyImpl implements Strategy { private ScheduledExecutor executor = null; @Override public boolean apply(..) { if (executor == null) { executor = Executors.new..().schedule..(); } // actual apply() logic } }

This way, the executor will get initialized the first time the strategy is applied, not sooner. And if SCHED is never used, the executor is never created.

However, I don't want to waste cycles checking for (executor == null) every single time SCHED is apply()ed, when I know the condition will always be false except for the first time.

To avoind doing so, this is what I came up with:

class ExecutorStrategyImpl implements Strategy { private ScheduledExecutor executor = null; private Function<StratParam, Boolean> normal = param -> { // normal apply() logic }; private Function<StratParam, Boolean> firstTime = param -> { innerStrategy = normal; if (executor == null) { executor = Executors.new..().schedule..(); } return normal.apply(param); } private volatile Function<StratParam, Boolean> innerStrategy = firstTime; @Override public boolean apply(StratParam param) { return innerStrategy.apply(param); } }

The reasoning behind the above code is as follows:

  1. innerStrategy initially points to firstTime lambda, which includes the executor initialization code.
  2. The first thread to get to apply() invokes innerStrategy.apply() and thus firstTime functionality.
  3. innerStrategy is changed to normal; volatile keyword ensures that this change will be visible for every other thread that comes after.
  4. Executor is initialized.
  5. Every other thread calling innerStrategy.apply() gets normal functionality instead.

My question is: is there a window between 2. and 3. where two threads may read innerStrategy with the same firstTime value, thus creating two executors? Or is volatile enough to prevent that?

And, of course: is there a better way to do this?

Improve this question
4
  • 1
    Could you clarify why it is a problem for the scheduler to be initialized even if it is not used, and why it is a problem to check whether it is null every time the apply method is called? Commented Feb 28, 2022 at 15:04
  • @VincentSavard Performance reasons. Yes, I know the load imposed by a running scheduled executor is low, and that of an additional if is negligible, but... If there's a way to eliminate both, I'd like to try it. Commented Feb 28, 2022 at 15:12
  • 1
    Did you measure it? Commented Feb 28, 2022 at 15:14
  • @VincentSavard My question is not about performance. If I had an actual, measurable performace problem I would have asked about it on StackOverflow, not here. But that is not the case: this code has not even been run yet (well... not in a multithreaded environment at least). My question to the Software Engineering community is whether this "volatile lambda" logic would ensure run-exactly-once behavior in a multi-threaded environment. Take it as a theoretical question if you wish. Commented Feb 28, 2022 at 15:32

3 Answers 3

Reset to default
0

Yes, multiple threads could begin executing firstTime.apply() before innerStrategy is changed. Those multiple threads could also observe executor == null to be true and thus create multiple executors (and incidentally your second example also suffers from that issue).

There are also other failure modes that potentially have a greater impact than that, for example:

  1. One thread changes innerStrategy.
  2. Second thread begins running normal.
  3. Second thread sees executor as null.
  4. First thread initialises executor.

To resolve these issues without a wider change in strategy, consider:

  1. Wrapping the initialisation code (including replacing innerStrategy, and the executor null check and initialisation) in a synchronized block.
  2. Replacing innerStrategy after initialising executor.
  3. Making executor volatile.

With that, firstTime.apply() may be entered by multiple threads but only one will enter the if block.

That should improve things but I'm not going to claim it resolves all failure modes. I don't have the exact quote to hand but Java Concurrency in Practice by Brian Goetz recommends against relying on complex reasoning to ensure things are thread safe; instead, use a simpler less performant approach until it's established that it's not performant enough for your requirements.

Simpler approaches I would pursue before taking this approach in reality:

  • Allowing the executor to exist even if you don't use it
  • Instead of an enum, can the caller provide an instance of the strategy?
  • Having a separate initialise() method on the strategy
Improve this answer
1
  • So, wrapping up: 1. No, this "volatile lambda" scheme won't work. Noted. 2. Re: executor being null for threads in apply(): that's not a problem. The executor just consumes from an existing queue, it's never accessed directly, so no problem if it's created later. I'm more worried about several threads creating several executors. 3. A synchronized block would definitely work, yes; however, it makes my admittedly complex code even more complex. You're right that, in this case, it's probably not worth it; so I've decided to just let the executor be. +1. Commented Mar 1, 2022 at 9:00
0
  1. I bet if the getInjector() returns a proper DI container the getInstance method should be thread-safe and will instantiate ScheduledStrategyImpl only once. Have you tried something like this?
 private enum Strats { NOOP (NoopStrategyImpl.class), REAL (RealtimeStrategyImpl.class), SCHED (ScheduledStrategyImpl.class), ; private final Class<? extends Strategy> strategyClass; Mode(Class<? extends Strategy> strategyClass) { this.strategyClass = strategyClass; } public boolean applyStrategy(StratParam param) { return getInjector() .getInstance(this.strategyClass) .apply(param); } }
  1. Have you thought of using Initialization on demand holder idiom?
Improve this answer
4
  • Yes, you're right that I could rely on Guice creating exactly one instance (as long as I use Singleton scoping) to ensure that the executor will be initialized exactly once; I haven't thought of that. That being said, the internal Guice logic for getInstance() includes several steps and checks, even when the instance has already been created. Given that I made this whole "contraption" just to avoid an additional if in my apply() method 😅 replacing said if for the whole of getInstace() is not really an option. Commented Mar 1, 2022 at 9:32
  • Re: holder idiom: I know the pattern, but I'm not sure it applies to my case, as I don't need to "getExecutor()" inside apply(), I just need to create it. But I see how I could adapt it, in a way, to achieve what I want. +1 anyway. Commented Mar 1, 2022 at 9:33
  • 1
    I didn't explain myself clearly. From your use of enums + getInjector().getInstance() I assumed you were trying to achieve sort of a singleton for your strategies. So my thinking was: instead of relying on a DI container, you can use this idiom. Then, in the apply method do something like strategyClass.getInstance(). I think that could be good trade-off. Commented Mar 1, 2022 at 17:52
  • Ah, I see. Create the holders inside the enum. That would ensure a single instance for each strategy, a lazy run-once initialization, and avoid any additional injector logic. Looks good! Commented Mar 2, 2022 at 10:07
0

volatile, besides that marks a variable that has to be used without copying it that empirically is the default behaviour even without using volatile, is useful in the context of 32 bits address bus and 64 bits data types, the way long and double are, since it acts the same synchronized acts for code blocks, adding atomicity to 64 bits operations.

The described scenario doesn't need synchronisation. The Executor lazy initialisation could be factored out in a factory class and implemented using class initialiser (static initialisers) that is guaranteed by the JVM specification to run just once when the class is loaded by the class loader...

class ExecutorFactory { private static class ExecutorHolder { public static final ScheduledExecutor EXECUTOR = Executors.new..(); } public static Executor get() { return ExecutorHolder.EXECUTOR; } }

...this way the executor is initialised with the first call to the EXECUTOR constant ExecutorHolder.EXECUTOR, that is with the first call to the ExecutorFactory.get() method.

Improve this answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.