reactor-core icon indicating copy to clipboard operation
reactor-core copied to clipboard

Review Scannable RUN_STYLE returned values

Open chemicL opened this issue 3 years ago • 4 comments

Some operators report incorrect RUN_STYLE from the Scannable::scanUnsafe(Attr key) implementations.

These incorrect responses can (and most probably do) cause errors in usages that scan the reactive pipeline to infer asynchronous behaviour.

The list can be completed during the actual review and fixed:

  • [ ] reactor.core.publisher.FluxOnBackpressureBuffer.BackpressureBufferSubscriber has an async boundary - a queue which can be drained from different thread than the publishing, therefore it should report ASYNC instead of SYNC.
  • [ ] reactor.core.publisher.FluxBufferBoundary and it's Main - the signals from "other" are an async boundary, so this operator should report ASYNC instead of SYNC.
  • [ ] ...

chemicL avatar Dec 12 '22 16:12 chemicL

@simonbasle any memories of why those operators (which could introduce thread boundary) are marked as SYNC in terms of run style?

according to the attribute docs:

public enum RunStyle {
			/**
				no guarantees can be given on the running mode (default value, weakest level of guarantee)
			 */
			UNKNOWN,

			/**
			 	the operator may change threads while running
			 */
			ASYNC,

			/**
			 	guarantees the operator doesn't change threads (strongest level of guarantee)
			 */
			SYNC;
		}

which means that the majority of operators that are marked as SYNC are ASYNC because they store values from upstream on Thread A but downstream Thread B which sends subscription.request may start draining immediately. - So this introduces a Thread Boundary

OlegDokuka avatar Dec 12 '22 17:12 OlegDokuka

This is important since the picture of the pipe and places where decoration should be applied is incorrect (e.g. sleuth https://github.com/spring-cloud/spring-cloud-sleuth/blob/3.1.x/spring-cloud-sleuth-instrumentation/src/main/java/org/springframework/cloud/sleuth/instrument/reactor/ReactorHooksHelper.java#L30)

cc @marcingrzejszczak

OlegDokuka avatar Dec 12 '22 18:12 OlegDokuka

IIRC the attribute value was affected on the basis of whether or not it explicitly changes threads, as in using a Scheduler.

simonbasle avatar Dec 13 '22 07:12 simonbasle

@simonbasle description of the ASYNC attribute sounds like the operator may potentially change the thread. Although they are not changing the thread explicitly they could be a cause of thread boundary

OlegDokuka avatar Dec 13 '22 10:12 OlegDokuka

Closing as invalid. It doesn't look like we have strong proof that this is a bug with @simonbasle note on the background behind ASYNC. In case a need arises we can come back to this.

chemicL avatar Mar 18 '24 12:03 chemicL