for #1102 Detect some blocking APIs inside parallel and single Scheduler

[simonbasle]

• extracted an abstract, externally reusable ThreadFactory
• added a NonBlocking marker interface
• added a hook in Schedulers that allows to also decide a Thread is NonBlocking even if you don't have a hand in creating it.
• block, blockOptional, blockFirst, blockLast, toIterable and toStream all check if the executing thread is NonBlocking and throw if it is
• reference documentation

[smaldini]

Nice, ny current comments:

• I would probably package scope the Abstract class for now, EventLoop is a special case anyway but I'd rather use a Schedulers.nonBlockingFactory(ThreadFactory) delegate in that case vs a a base API. Such util could be reused by Netty for instance to mark its own threadFactory with NonBlocking thread
• We could hide NonBlocking as well given Schedulers.isInNonBlockingThread() : boolean
• I don't think we need the extra predicate for now, I don't want to over expand beyond basic help for blocking scenarios. But that's open to discuss, I just prefer to start pretty basic for now :)

[rstoyanchev]

Neat, is it possible to use this in the context of HTTP request processing, i.e. not using any scheduler explicitly, but simply processing in a Reactor Netty thread?

[simonbasle]

@rstoyanchev detection is constrained to (and initiated by) a known set of operators: block() variants and toIterable()/toStream(). It wouldn't detect eg a Thread.sleep inside a map. Don't know if that helps in your case.

[simonbasle]

@smaldini

• I don't think the delegating pattern is very well suited for this ThreadFactory where we'd need to wrap the Thread. Since Thread isn't an interface, we'd have 2 instances and we'd need to wrap every single method of Thread to delegate to the original one.
• given the above, I think that at least keeping the NonBlocking marker interface is a must.
• The rationale for the extra predicate is that with some libraries / platforms like Android, you cannot change the Thread implementation or ThreadFactory used. That additional predicate still gives you an opportunity to detect these threads and consider them blocking-incompatible.

[rstoyanchev]

@simonbasle yes I meant using one of the block() variants but on the Netty event loop (i.e. while handling a request in Reactor Netty).

[simonbasle]

@rstoyanchev yeah with the current state reactor-netty could be changed to instantiate Thread that implement NonBlocking and that would help detect such cases. Or maybe the use in reactor-netty alone would justify making the ReactorThreadFactory public. Either way I think it would help with your use case.

[codecov-io]

Codecov Report

Merging #1109 into master will increase coverage by 0.56%.
The diff coverage is 93.18%.

@@             Coverage Diff              @@
##             master    #1109      +/-   ##
============================================
+ Coverage     83.72%   84.28%   +0.56%     
- Complexity     3546     4200     +654     
============================================
  Files           334      337       +3     
  Lines         27677    28688    +1011     
  Branches       5183     5261      +78     
============================================
+ Hits          23172    24180    +1008     
- Misses         2951     2955       +4     
+ Partials       1554     1553       -1

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/reactor/core/scheduler/Schedulers.java	85.71% <100%> (-0.74%)	55 <5> (+3)
...actor/core/publisher/BlockingSingleSubscriber.java	77.35% <100%> (+1.84%)	19 <8> (+2)	⬆️
...ore/src/main/java/reactor/core/publisher/Flux.java	99.85% <100%> (-0.02%)	917 <5> (+418)
...java/reactor/core/scheduler/ParallelScheduler.java	78.26% <100%> (ø)	24 <0> (ø)	⬇️
...core/publisher/BlockingOptionalMonoSubscriber.java	92.06% <100%> (+0.53%)	25 <8> (+2)	⬆️
.../java/reactor/core/scheduler/ElasticScheduler.java	83.73% <100%> (ø)	26 <0> (ø)	⬇️
...n/java/reactor/core/scheduler/SingleScheduler.java	77.08% <100%> (ø)	17 <0> (ø)	⬇️
...a/reactor/core/scheduler/ReactorThreadFactory.java	85.71% <85.71%> (ø)	6 <6> (?)
...ava/reactor/core/publisher/EventLoopProcessor.java	79.79% <0%> (-1.04%)	51% <0%> (-2%)
...java/reactor/core/publisher/FluxBufferTimeout.java	72.02% <0%> (-0.7%)	3% <0%> (ø)
... and 12 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74b94a4...3c2a760. Read the comment docs.

[smaldini]

I would just get rid of the Schedulers extra method or eventually, change to isInNonBlockingThread() ?
I think instanceof should be enough and wouldn't require extra API for Schedulers.

[simonbasle]

@smaldini updated. is that what you had in mind? or maybe you were just talking about renaming the method variant that takes a Thread?