Fix termination of request context for non-blocking scheduled methods

[mkouba]

• fix the problem mentioned in
  Hibernate Reactive Panache MSSQL Native Build #23739 (comment)

[DavideD]

Nice, LGTM

[DavideD]

A test would be nice though. It's easy to mess up this stuff with a refactoring.

[mkouba]

A test would be nice though. It's easy to mess up this stuff with a refactoring.

For sure. We'll need an integration test for this...

[DavideD]

By the way, the fix looks correct but I haven't run a test to check that it fixes the issue. I trust you on this.

[mkouba]

By the way, the fix looks correct but I haven't run a test to check that it fixes the issues. I trust you on this.

I did. But since CP is involved one can be never sure ;-).

[mkouba]

I will add a test shortly...

[mkouba]

In fact, the fix is probably wrong. Or at least does not cover all possible cases. I'm on it.

[mkouba]

The PR should be ready for review now. I've modified the existing NonBlockingScheduledMethodTest to verify the request context termination. Note that we don't need to terminate the context when an invokeBean() invocation results in an exception because the failure is wrapped in the generated invoker class with CompletableFuture.failedStage().

[DavideD]

Are you sure this is right?

The context could be deactivated before being used by invokeBean.

[mkouba]

Yes, deactivation means that we remove a threadlocal but do not destroy the context/beans, i.e. the thread is "clear". If Mutiny is used inside the invokeBean() then we rely on context propagation to make a snapshot of the current context and propagate it accordingly.

It wouldn't work for CompletionStage with a custom executor though. In this case, a user needs to handle the context propagation manually (as it was before).

[manovotn]

Looks correct, CP takes snapshot based on the ContextState of currently active context when it is captured. If I get this scenario right, then this will happen when you perform invokeBean().

[DavideD]

Note that we don't need to terminate the context when an invokeBean() invocation results in an exception because the failure is wrapped in the generated invoker class with CompletableFuture.failedStage().

There's nothing in the javadoc of invokeBean that states that it should never throw an exception though. It actually declares throws Exception in the signature.

[DavideD]

if the implementation of invokeBean looks something like this:

protected CompletionStage<Void> invokeBean(ScheduledExecution execution) throws Exception {
    if (validationFails()) {
        throw new RuntimeException("Validation failed!");
    }
    return  doSomething()
}

Will everything still work when the validation fails?

Maybe I'm missing something though. My review is only based on the code in the PR.

[DavideD]

Maybe I'm missing something though. My review is only based on the code in the PR.

In particular, I don't know when we have to deactivate or terminate the context and in which order these operations can run.

[mkouba]

if the implementation of invokeBean looks something like this:

protected CompletionStage<Void> invokeBean(ScheduledExecution execution) throws Exception {
    if (validationFails()) {
        throw new RuntimeException("Validation failed!");
    }
    return  doSomething()
}

Will everything still work when the validation fails?

Maybe I'm missing something though. My review is only based on the code in the PR.

The actual (generated) implementation of the ScheduledInvoker for Jobs.everySecondUni() looks like:

public class NonBlockingScheduledMethodTest$_Jobs_ScheduledInvoker_everySecondUni_bc4773d4b48cf55d38fb70c74dd8ac9a99dcf2af
		extends
			DefaultInvoker {
	public CompletionStage invokeBean(final ScheduledExecution scheduledExecution) throws Exception {
		try {
			final ArcContainer container = Arc.container();
			return ((NonBlockingScheduledMethodTest$Jobs) container
					.instance(container.bean("43b9ee810358bfaef695a734f2ffc1709e5a4cfb")).get()).everySecondUni()
					.subscribeAsCompletionStage();
		} catch (Throwable ex) {
			return CompletableFuture.failedStage(ex);
		}
	}

	public boolean isBlocking() {
		return false;
	}
}

[mkouba]

@manovotn pls could you review this one. Your knowledge of CDI and CP could be beneficial here.

[mkouba]

Note that we don't need to terminate the context when an invokeBean() invocation results in an exception because the failure is wrapped in the generated invoker class with CompletableFuture.failedStage().

There's nothing in the javadoc of invokeBean that states that it should never throw an exception though. It actually declares throws Exception in the signature.

I've removed the Exception from the signature (probably a leftover). Also note that the DefaultInvoker is not part of the public API.

[manovotn]

LGTM, this way the context should get destroyed correctly.

[DavideD]

Nice, thanks!