Reactive `HandlerMethodArgumentResolver.resolveArgument` should return `Mono<@Nullable Object>`

[rwinch]

The javadoc for HandlerMethodArgumentResolver.resolveArgument states

Mono for the argument value, possibly empty

The statement possibly empty means that the type is @Nullable, so the return type should align with this by returning Mono<@Nullable Object>.

[sdeleuze]

I think Mono parameterized type by design can't be null, so I am not sure we can make that equivalence. Here I tend to think we just hint that the mono can be empty and return no value. I will double check with the implementations.

[rwinch]

I think Mono parameterized type by design can't be null, so I am not sure we can make that equivalence. Here I tend to think we just hint that the mono can be empty and return no value. I will double check with the implementations.

Now I think I see. Rather than T being nullable, methods like block() are marked as @Nullable T. This seems to force the type of T to be non-null, but it is limiting in that typing cannot guarantee that methods like block() are non-null based upon the generic type passed into Mono. This is often not a problem, but in a servlet application using WebClient to aggregate multiple REST APIs concurrently, it is not uncommon for the block method to be called.

With the current null semantics of Mono, I think it makes sense that Mono<Object> is the return type. However, I wonder if this is actually the best way to represent nullability Mono due to its limitations I mentioned above.

[sdeleuze]

Indeed we can't guarantee that via the nullability at type level for Mono (partially because Reactor made the choice to use the same type for potentially empty and never empty single values), but block() return value is @Nullable T (BTW be aware I will start migrating reactor-core nullability annotation to JSpecify with @chemicL shortly) so you have the information it can be null, so checks or assertions should be always performed. Here the Javadoc just put more focus on this fact that empty use case will happen here, so you should managed it (that also applies to use cases not involving nullability).

[chemicL]

The JSpecify effort is my top priority right now and I'm on it.

Regarding the nullability aspect vs Mono having a single-or-empty semantics... Even if we had Single to represent "exactly one item or error" and Completable to represent "completion or error", we'd face the same exact issue in the case of Flux. There we have for instance @Nullable public final T blockLast() (or public final @Nullable T blockLast() with JSpecify), where the nullability of T is orthogonal to the nullability of the return value of block*() methods which combine together the act of waiting for a value or completion and have the need to express both, hence @Nullable return type.

[rwinch]

@chemicL I'm not sure that I understand the case for @Nullable T block(). Is there a good place to move this discussion since it is more related to Reactor at this point?

[sdeleuze]

@rwinch I can maybe add a bit of context before continuing potentially the discussion on Reactor side. For simple types with consistent nullability, it makes sense to define nullability at generic type parameter level. On use-site, List<@Nullable T> contains nullable elements, List<@NonNull T> or List<T> in a null-marked context contains non-nullable elements, same principe for Supplier, Consumer, etc.

Reactor Mono and Flux are different beasts, they are mostly dealing with signals and accepting nullable values in very specific methods, usually designed to be bridged with the blocking world (also to implement fusion but that's another story), where the absence of onNext signal translates to null. That's why at type level, we keep a non-nullable T at type level (both at declaration-site and use-site) and specify nullability at method level where that make sense (@Nullable T block() is a good example of that).

Hope this helps.

[chemicL]

@rwinch Feel free to open a dedicated reactor-core issue if needed, but maybe with @sdeleuze explanation it will be unnecessary. Also, to extrapolate the answer, let's for a moment move away from Reactor and think of Queue<T>.

// T is null-marked so it's @NonNull
Queue<T> queue = new LinkedList<>();
assertThat(queue.poll()).isNull();

The outcome of poll() for an empty queue is null since the queue has no elements, despite the fact that elements themselves can not refer to null.

It's the same case for Flux<T> and Mono<T>.

Flux<T> flux = Flux.empty();
assertThat(flux.blockLast()).isNull();

Flux<T> mono = Mono.empty();
assertThat(mono.block()).isNull();

This is the result of a Publisher delivering onComplete() that was not preceded by an onNext(), similarly as poll() for an empty queue yields null.
In the case of onError(), block*() would simply throw a RuntimeException so there's no annotation for that but it's also a possible outcome decoupled from the nature of the elements of type T.

[rwinch]

@chemicL How do I properly represent this scenario?

// documented as guaranteed not to return an empty Mono
public static Mono<String> getNonEmpty() {
	Mono<String> result = getResult();
	return result.switchIfEmpty(Mono.error(new RuntimeException("Cannot be empty")));
}

public static void main(String[] args) {
	String cannotBeNull = getNonEmpty().block(); // cannot return null
	nonNullArg(cannotBeNull); // Fails, but I know that cannotBeNull is a non-null value!
}

public static void nonNullArg(String arg) {
}

[chemicL]

@rwinch I suppose this is an open question :(

In the reactor-core codebase I am consequently adding @SuppressWarnings("DataFlowIssue") to methods that do this to avoid NullAway warnings (with a NullAway configuration that allows treating DataFlowIssue as NullAway suppression). For my IDE (Intellij IDEA) what would be enough is to add assert cannotBeNull != null but for NullAway it's not doing the trick, hence the suppression. Another way to approach it would be to add a proxy method with a @Contract annotation (as discussed in this here) but I am not convinced.

@sdeleuze do you have a different recommendation?

---

I wish in reactor-core we could return something more specific, e.g. in the case of Mono#single() its return type could be a SingleMono that overrides block() with a @NonNull return value (just like MonoSingleCallable currently does). Unfortunately, we're not at the liberty to do that for elaborate reasons (operator assembly wraps these specialized types with a generic one for multiple purposes), so we have to rely on the generic Mono#block contract.

---

But back to the queue example, this problem is independent of reactor-core:

	public static void main(String[] args) {
		Queue<String> queue = createQueue();

		printString(queue.poll()); // fails, but I know it's not null
	}
	
	static Queue<String> createQueue() {
		Queue<String> queue = new LinkedList<>();
		queue.add("hello");
		return queue;
	}

	static void printString(String s) {
		System.out.println(s);
	}