Provide BlockHound integration. #1637
Conversation
only be used there.
johnrengelman
force-pushed
the
blockhound
branch
from
c658187
to
5576f74
Compare
|
I'm back and forth on where this class should actually exist. It probably shouldn't be used in production code, but maybe someone chooses to do that on purpose in like a test environment or something. |
| builder.allowBlockingCallsInside("io.netty.channel.pool.SimpleChannelPool", "close"); | ||
| builder.allowBlockingCallsInside("io.netty.channel.pool.FixedChannelPool", "close"); | ||
| builder.allowBlockingCallsInside("ratpack.exec.internal.DefaultExecController", "close"); | ||
| builder.allowBlockingCallsInside("ratpack.test.internal.BlockingHttpClient", "request"); |
There was a problem hiding this comment.
this is a very dangerous method to be allow-listed IMO, especially from the default integration.
There was a problem hiding this comment.
The BlockingHttpClient one? That is only used for testing applications and not in production code.
the thing here is that it needs to block in order to do the async->sync transition in your tests.
There was a problem hiding this comment.
Judging by Netty, reactor-netty, reactor-kafka, and a number of other async non-blocking projects that integrated BlockHound, I can confidently say that testing of async code can be done without blocking calls in non-blocking threads, so maybe BlockHound is a red herring here?
There was a problem hiding this comment.
Could be. I’ll dig deeper into this. Looking at it this should be called from the junit thread. I suspect maybe there is a test that is doing something wrong.
thansk for the feedback!
There was a problem hiding this comment.
Happy to help! That was my thinking as well - it is common to have blocking calls while testing non-blocking code, just from blocking-friendly threads. We were able to identify a couple of incorrectly handled context switches in reactor-kafka when we added BlockHound, where we assumed that it was safe to block, but apparently not. Your situation sounds very similar, and I hope it will be easy to identify the problematic code block 👍
There was a problem hiding this comment.
See https://github.com/reactor/BlockHound/blob/master/docs/tips.md for some tips on how to debug, btw
There was a problem hiding this comment.
Also, you can implement "record & report" vs "fail fast". See an example here:
https://github.com/reactor/reactor-kafka/pull/75/files#diff-7158e97462adbe44834fa2d106d27922da1f3407c75ece16e47ca7f9443a3a07R105
| builder.allowBlockingCallsInside("ratpack.test.internal.BlockingHttpClient", "request"); | ||
|
|
||
| builder.nonBlockingThreadPredicate(current -> | ||
| t -> Execution.isManagedThread() ? Execution.isComputeThread() : current.test(t) |
There was a problem hiding this comment.
My recommendation would be to have Execution.isManagedThread and Execution.isComputeThread accept Thread instances and provide overloads that use Thread.currentThread()
|
@johnrengelman, anything blocking this PR? |
Closes #1526
This change is