Fix Mono.awaitSingleOrNull non-compliance with 2.7

[dkhalanskyjb]

No description provided.

[dkhalanskyjb]

Filed an issue to Reactor about this, as, after some research, it does look like a clear-cut case of non-compliance on their part: reactor/reactor-core#3117 However, currently, we're also non-compliant. I propose that we fix and merge it without testing, so that, when a Reactor version with the fix is available, users of our code obtain compliant behavior from us immediately.

[OlegDokuka]

Just FYI. Having cancel racing with request is expected behavior as of rule 2.7. None of the existing Reactive libs doing serialization for those calls and that is specification expected. Please see related PR I have done in the past for the spec see reactive-streams/reactive-streams-jvm#489. I guess if we have doubts about the rule, let's start from the spec first an make more clarification to the rule hint so all vendors can fix that problem

[dkhalanskyjb]

@OlegDokuka, the interpretation that 2.7 means that request can not race with request, and cancel can not race with cancel doesn't seem plausible, because https://github.com/reactive-streams/reactive-streams-jvm#3.5 says that cancel must be prepared for such racing calls. So, by your interpretation, only request is not allowed to race with request. But then, why does rule 2.7 mention cancel at all? I think the interpretation that cancel must not race with request is entirely sensible. If this was not the intent behind the rule, then yes, I agree that clarifying it is a great idea.

[OlegDokuka]

the interpretation that 2.7 means that request can not race with request, and cancel can not race with cancel doesn't seem plausible, because reactive-streams/reactive-streams-jvm#3.5 says that cancel must be prepared for such racing calls

@dkhalanskyjb rule 3.5 is not about race conditions but rather about idempotency (if canceled once, the following cancelations should have no effect on the state) and non-blocking behavior (the caller thread MUST not be blocked waiting for other thread or stolen by CPU-intensive work). That means the cancel method MUST not perform resource cleanup so the caller thread is blocked by that work. If you look at the implementation of libs like Reactor | RxJava, you will find that all of them just set the volatile boolean canceled to true and that it (best effort).

So, by your interpretation, only request is not allowed to race with request. But then, why does rule 2.7 mention cancel at all? I think the interpretation that cancel must not race with request is entirely sensible. If this was not the intent behind the rule, then yes, I agree that clarifying it is a great idea.

it makes no sense to have such behavior since .request(n) may end up being blocked (no rules mandate it being non-blocking, only suggests) so the serialized cancel will never be delivered (serial behavior mandates non-overlapping calls), hence functionality like timeout broken. Apart, the majority of operators will get overcomplicated implementation which is huge maintenance work and likely to decrease overall performance.

---

cancel call MUST be independent from the request call and MUST have a possibility to race with the request. Having separately serial guarantee between requests and cancels calls allows simplification of intermediate operators since they would know that modification within request will have serial guarantees, hence eliminating the need for stronger memory management for "method local mutations" (e.g. -> https://github.com/reactor/reactor-core/blob/main/reactor-core/src/main/java/reactor/core/publisher/FluxSwitchOnFirst.java#L680)

[dkhalanskyjb]

rule 3.5 is not about race conditions

Could you clarify this? I'm not sure that it's not also about race conditions. In particular,

MUST be thread-safe

where thread-safety is defined as

Can be safely invoked synchronously, or asychronously, without requiring external synchronization to ensure program correctness.

in addition to idempotence seems to imply that it the subscriptions must be prepared for several calls to cancel to happen in parallel (thread safety) and ensure that only one of them has an effect (idempotence).

[OlegDokuka]

@dkhalanskyjb indeed, it sounds like about racing but the intent is different I guess we can clarify it in the spec. Also, I suggest paying more attention to the hint which aims to explain a dedicated rule in deeper detail and clarify its wording.

The intent of this rule is to establish that cancel is intended to be a non-obstructing method, and should be as quick to execute as possible on the calling thread, so avoid heavy computations and other things that would stall the caller´s thread of execution. Furthermore, it is also important that it is possible to call it multiple times without any adverse effects.

---

in addition to idempotence seems to imply that it the subscriptions must be prepared for several calls to cancel to happen in parallel (thread safety) and ensure that only one of them has an effect (idempotence).

Not really, thread-safe in that context means to apply cancellation in the way so the main producing process will observe it. In practice, data generation happens inside request() method and on the request caller thread. To have the cancel call being propagated it has to be synchronous to the request() caller thread.

Here, Meaning happening on the same thread as request e.g:

   Thread A: request() -> onNext() -> cancel()

or, asynchronous - which means another concurrent thread is calling cancel while request() is still generating data. E.g.

Thread A: request() -> onNext() -> onNext()

Thread B:        cancel()

The above can be easily derived and concluded from the term Thread-safe which is clarified in the vocabulary - https://github.com/reactive-streams/reactive-streams-jvm/#term_thread-safe

---

Besides, I will create a separate PR to clarify rules 3.5 as well as 2.7 so it is going to be easier to understand

[OlegDokuka]

@qwwdfsad this PR is not really relevant, so my suggestion is to reject it.

[dkhalanskyjb]

I think we'll keep this open as a reminder until the spec is changed.

[dkhalanskyjb]

Hey @OlegDokuka, any progress on updating the spec? I don't see any new proposals in https://github.com/reactive-streams/reactive-streams-jvm/pulls — is that the place to look?