Provide blockhound integrations #4493
Conversation
|
Note: There is a known issue where a |
jrhee17
force-pushed
the
feature/blockhound-integration
branch
from
90e89e3 to
11e8e18
Compare
|
As of Netty 4.1.85, FastThreadLocalThread permits to perform blocking calls. If the flag is enabled, the Netty Blockhound integration module does not assume the thread is a non-blocking thread. netty/netty#12978 |
|
Hi, let me report the following after trying blockhound with Armeria 1.20.3 Zipkin tracing can block when flushing spansThough it sounds ignorable, I'm not sure if armeria integration should be the one ignoring it, as opposed to each application using it. Reading Prometheus gauges (ExecutorServiceMetrics) can blockThis looks ignorable. recording in a Hdr histogram can cause a sleepThe sleep is about 1ms at most, so I suppose it can be ignored, but let's be aware that it could happen for each histogram that is being updated in a non-blocking thread. Submitting a task to a blocking scheduler can blockThe lock seems like it's never held for long. Guava's MapMaker's ConcurrentMap can blockAgain, the lock seems never held for long. |
|
Is there any progress on this? |
Let me try working on this again when I have more time 😅 At the moment, the difficult part is figuring out why failed tests are failing. The pattern most often found is:
Let me look into this again next week 😅 Sorry about the delay |
|
If we introduce a general interface that marks all methods of its subclasses are safe to execute in an event loop, it would be easier to implement the BlockHound integration module. The following example was borrowed from the change in #4837 public interface EventLoopSafe {}
class DestroyHandler implements BiFunction<Object, Throwable, Void>, EventLoopSafe {
@Override
public Void apply(Object unused, Throwable throwable) {
// BlockHound will report this blocking operation without EventLoopSafe
lock.lock();
try {
...
} finally {
lock.unlock();
}
return null;
}
} |
I'm not sure if I understood the intention, but could it be easier if we just introduce a lock which is ignorable by Blockhound? |
|
In #4837, it is likely that Question: Is it feasible to extend class ReentrantShortLock extends ReentrantLock { ... }
BlockHound.Builder builder = ...;
builder.allowBlockingCallsInside("com.linecorp.armeria.internal.common.ReentrantShortLock",
"lock"); |
jrhee17
force-pushed
the
feature/blockhound-integration
branch
from
ddbdbc3 to
9967be7
Compare
|
Sorry about the delay 🙏 I believe this PR is (finally) ready for review |
brave/src/main/resources/META-INF/services/reactor.blockhound.integration.BlockHoundIntegration
Outdated
Show resolved
Hide resolved
| public void applyTo(Builder builder) { | ||
| // short locks | ||
| builder.allowBlockingCallsInside("com.linecorp.armeria.client.HttpClientFactory", | ||
| "pool"); |
There was a problem hiding this comment.
Could you explain which part of the method is a blocking call? 🤔
There was a problem hiding this comment.
It seems like the report from @mauhiz contains the stacktrace. I was able to reproduce this failure from our CI as well.
at java.base/java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:267)
at com.linecorp.armeria.internal.shaded.guava.collect.MapMakerInternalMap$Segment.put(MapMakerInternalMap.java:1476)
at com.linecorp.armeria.internal.shaded.guava.collect.MapMakerInternalMap.putIfAbsent(MapMakerInternalMap.java:2419)
at java.base/java.util.concurrent.ConcurrentMap.computeIfAbsent(ConcurrentMap.java:331)
at com.linecorp.armeria.client.HttpClientFactory.pool(HttpClientFactory.java:404)
at com.linecorp.armeria.client.HttpClientDelegate.acquireConnectionAndExecute0(HttpClientDelegate.java:169)
| builder.allowBlockingCallsInside("org.HdrHistogram.ConcurrentHistogram", "getCountAtIndex"); | ||
| builder.allowBlockingCallsInside("org.HdrHistogram.WriterReaderPhaser", "flipPhase"); | ||
|
|
||
| // StreamMessageInputStream internally uses a blocking queue |
There was a problem hiding this comment.
It seems like it uses add instead offer? 🤔
There was a problem hiding this comment.
It seems like LinkedBlockingQueue internally delegates add to offer
https://github.com/openjdk/jdk/blob/f5a6b7f7c03c00c96d0055f9be31057675205e13/src/java.base/share/classes/java/util/concurrent/LinkedBlockingQueue.java#LL81C45-L81C58
https://github.com/openjdk/jdk/blob/f5a6b7f7c03c00c96d0055f9be31057675205e13/src/java.base/share/classes/java/util/AbstractQueue.java#L94
core/src/test/java/com/linecorp/armeria/client/Http1HeaderNamingTest.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/util/AbstractListenable.java
Show resolved
Hide resolved
core/src/main/java/com/linecorp/armeria/common/CoreBlockhoundIntegration.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public void applyTo(Builder builder) { | ||
| // UUID.randomUUID is called by default | ||
| builder.allowBlockingCallsInside("graphql.execution.ExecutionId", "generate"); |
There was a problem hiding this comment.
What do you think of removing this rule and using ctx.id().text() as the ExecutionId?
- I don't see that users would want to use the
UUID.randomUUIDfor the default execution ID. - It would be a better choice to correlate an HTTP request with a GraphQL query.
final ExecutionInput executionInput =
builder.context(ctx)
.executionId(ExecutionId.from(ctx.id().text()))
...We may add an API that takes an
ExecutionId factory from GraphqlServiceBuilder later.
There was a problem hiding this comment.
Created an issue for this. #4867
There was a problem hiding this comment.
Thanks! Seems like a simple enough workaround to include in this PR 👍
jrhee17
force-pushed
the
feature/blockhound-integration
branch
from
add4a98 to
7b6dd68
Compare
| * A {@link BlockHoundIntegration} for the brave module. | ||
| */ | ||
| @UnstableApi | ||
| public final class BraveBlockHoundIntegration implements BlockHoundIntegration { |
There was a problem hiding this comment.
Is there any case a user needs to refer to our BlockHoundIntegration implementations manually? If not, can we move all BlockHoundIntegration into the internal package, given that they are loaded automatically via SPI?
There was a problem hiding this comment.
I thought users may want to exclude some BlockHoundIntegrations (not necessarily armeria's)
e.g.
BlockHound.install(BlockHoundIntegration... integrations) - same as BlockHound.install(), but adds user-provided integrations to the list.
BlockHound.builder().install() - will create a new builder, without discovering any integrations.
You may install them manually by using BlockHound.builder().with(new MyIntegration()).install().
https://github.com/reactor/BlockHound/blob/master/docs/customization.md#customization
If there is no need, I'm fine with hiding them. Let me know what you think
There was a problem hiding this comment.
I see. Then it's fine as it is :-)
jrhee17
force-pushed
the
feature/blockhound-integration
branch
from
a0deb15 to
7b6dd68
Compare
|
Thank you! |
Motivation:
Armeria users often use
BlockHoundto determine whether there are any blocking calls in their applications.Providing a basic
BlockHoundintegration can help de-duplicate some code and debugging efforts.I propose the following:
BlockHoundintegration up-to-date with changes.ThreadPoolExecutor). As long as these locks are verified to be short-living, we can add it to our integration.MicrometerorHdrHistogramto provideBlockhoundintegrations. Since Armeria usesMicrometerto record metrics from event loops, I think it is reasonable that we addHdrHistogramrelated calls to ourBlockhoundintegration.Also note that there are some blocking calls within the codebase, but this PR attempts to just introduce blockhound rather than remove such calls.
Modifications:
BlockHoundintegrations forcore,brave,grpc,retrofit2,scala,sangriamodulesBlockHoundintegration which allows test-specific blocking callsblockhoundgradle property flag is specified, run tests withBlockHoundenabledBlockHound.installis not invoked and not enabled in the code-baseReentrantShortLockwhich Armeria'sBlockHoundwhitelists and change all usages ofReentrantLockBlockingFastThreadLocalThreadis now used for non-event loops since netty whitelistsFastThreadLocalThreadwithpermitBlockingCalls==trueShutdownSupport::shutdownis now called fromstartStopExecutor. Although this method returns a CF, the call can actually be blockingSTART_STOP_EXECUTORis now a non-eventloop daemon thread instead of using theGlobalEventExecutorGlobalEventExecutoris an event loop.BlockingUtils#blockingRunin case users want to run blocking calls from testskotlinx-coroutines-debugdependency so that CoroutinesBlockHoundintegration is loadedThreadFactorywhen creating aEventLoopExtensionorEventLoopGroupExtensionSamlService,SamlDecoratornow runs some calls from the blocking executorZookeeperEndpointGroupnow uses a non-event-loop thread factoryResult:
BlockHoundintegrations provided by armeria