make mark/sweep prepare async with main thread

[RatanRSur]

Fixed Issue(s)

fixes #917

[shemnon]

Should the whole block be in the execute runnable? i.e. add the compare and set check into the async part? Are we sure start will only ever be called once?

[RatanRSur]

I think we could call this as many times as we like and the compareAndSet would make sure that the if body is only executed once, right? That said, I don't mind pushing the whole thing into the execute. Works for me.

[shemnon]

The executor may back up, so we could queue up a bunch of the lambdas for execution before the first one starts (which is where I presume the stat gets set to running or prepare). While the test is idempotent the execution does not appear on its surface to be, especially since we are adding observers inside the async function.

[mbaxter]

The state variable is an AtomicReference - the code inside the block is guaranteed to execute at most once as it's currently written.

[mbaxter]

nm - I think I see the issue: the startup logic could actually run after the shutdown logic because you don't know when it will actually execute 👍

[shemnon]

Suggested change

	public void start() {
	if (state.compareAndSet(State.IDLE, State.RUNNING)) {
	LOG.info("Starting Pruner.");
	executorService = executorServiceSupplier.get();
	pruningStrategy.prepare();
	blockAddedObserverId = blockchain.observeBlockAdded(this::handleNewBlock);
	execute(
	() -> {
	pruningStrategy.prepare();
	blockAddedObserverId = blockchain.observeBlockAdded(this::handleNewBlock);
	});
	}
	}
	public void start() {
	execute(this::startAsync);
	}
	private void startAsync() {
	LOG.info("Starting Pruner.");
	executorService = executorServiceSupplier.get();
	pruningStrategy.prepare();
	blockAddedObserverId = blockchain.observeBlockAdded(this::handleNewBlock);
	pruningStrategy.prepare();
	blockAddedObserverId = blockchain.observeBlockAdded(this::handleNewBlock);
	}
	}

[RatanRSur]

@mbaxter I think you were the one who put the ExecutorService behind a supplier. Do you remember your rationale for that? I inlined it in this change so that we could run the start code on the executor itself.

[mbaxter]

@mbaxter I think you were the one who put the ExecutorService behind a supplier. Do you remember your rationale for that?

Yeah - I think the point of Supplier was to prevent the Pruner from spinning up resources before start is called. Otherwise, if you setup a single-threaded executor in the constructor, but never actually start the Pruner, the executor will never get shutdown because the stop method won't run.