Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide blockhound integrations #4493

Merged
merged 71 commits into from
May 25, 2023
Merged
Show file tree
Hide file tree
Changes from 68 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
aefb531
add blockhound on tests
jrhee17 Oct 20, 2022
d7f7832
add jvm args for >=13 only
jrhee17 Oct 20, 2022
efc103e
add basic integration, add failing tests
jrhee17 Nov 2, 2022
a79db10
fork FastThreadLocalThread
jrhee17 Nov 3, 2022
4ed4430
preload utilities
jrhee17 Nov 3, 2022
a525ea5
cleanup
jrhee17 Nov 3, 2022
a59abe9
more cleanups
jrhee17 Nov 3, 2022
a372f97
more cleanups
jrhee17 Nov 4, 2022
38eab8e
more cleanups
jrhee17 Nov 4, 2022
11e8e18
more cleanups
jrhee17 Nov 4, 2022
c19df94
more cleanups
jrhee17 Nov 4, 2022
de27a01
fix trace tests
jrhee17 Nov 4, 2022
915a13f
more cleanups, share startstop by default
jrhee17 Nov 4, 2022
4667dd7
more cleanups
jrhee17 Nov 4, 2022
4e347fe
more cleanups
jrhee17 Nov 5, 2022
4c33818
more cleanups
jrhee17 Nov 7, 2022
951d139
Merge branch 'main' of https://github.com/line/armeria into feature/b…
jrhee17 Apr 26, 2023
e7b5f63
upgrade blockhound
jrhee17 Apr 26, 2023
1c12c96
introduce ReentrantShortLock
jrhee17 Apr 26, 2023
9967be7
more cleanups
jrhee17 Apr 26, 2023
ac65758
test fix
jrhee17 Apr 27, 2023
bcb8852
failing test
jrhee17 Apr 27, 2023
fa0af51
more test failures
jrhee17 Apr 27, 2023
f63fb1d
more rules
jrhee17 Apr 27, 2023
4868630
more tests
jrhee17 Apr 27, 2023
963dbff
Merge branch 'main' into feature/blockhound-integration
jrhee17 Apr 28, 2023
945b97b
more failures
jrhee17 Apr 28, 2023
2c35237
more fixes
jrhee17 Apr 28, 2023
c090caa
lint
jrhee17 Apr 28, 2023
72f36c9
more rules
jrhee17 Apr 28, 2023
112bf0f
test failure
jrhee17 Apr 28, 2023
7886cdf
more failures
jrhee17 Apr 28, 2023
9efa8fc
upload blocked stacktraces
jrhee17 May 2, 2023
a954fb7
lint
jrhee17 May 2, 2023
94709eb
remove failing test
jrhee17 May 2, 2023
16b981d
cleanups
jrhee17 May 2, 2023
c53d47e
fix build ci
jrhee17 May 2, 2023
91817ee
more cases
jrhee17 May 2, 2023
1306226
reorganize
jrhee17 May 2, 2023
8d018c8
add javadocs
jrhee17 May 2, 2023
c3a6751
always let users know whether a block was detected
jrhee17 May 2, 2023
ee70fe7
more fixes
jrhee17 May 2, 2023
de28584
fix to exit with status code
jrhee17 May 2, 2023
f8378d6
add debug logs
jrhee17 May 3, 2023
7c45b4f
explicitly use if statements
jrhee17 May 3, 2023
83b9dea
specify shell
jrhee17 May 3, 2023
9fc4de7
minor cleanups
jrhee17 May 3, 2023
ceb90c3
allow write to file
jrhee17 May 3, 2023
c728c0d
lint
jrhee17 May 3, 2023
790a159
minor fixes
jrhee17 May 3, 2023
7bce4ed
address comment by @trustin
jrhee17 May 3, 2023
d62510a
correct the upload name
jrhee17 May 3, 2023
ece0c57
more fixes
jrhee17 May 3, 2023
1964164
Merge branch 'main' into feature/blockhound-integration
jrhee17 May 3, 2023
af3ed91
just ignore blocking queue
jrhee17 May 3, 2023
1b83ac5
failing test
jrhee17 May 4, 2023
b533ab5
run blockhound on 19 only
jrhee17 May 4, 2023
13fd9ad
minor fix
jrhee17 May 4, 2023
eb3b520
correct comment
jrhee17 May 4, 2023
30a8e9e
minor nits
jrhee17 May 4, 2023
80c6769
address comments by @minwoox, @ikhoon
jrhee17 May 10, 2023
9d2ea23
remove copyright
jrhee17 May 10, 2023
18a99f5
fix checkstyle failure
jrhee17 May 10, 2023
622ee84
lint
jrhee17 May 10, 2023
3587660
nit
jrhee17 May 10, 2023
957e9c5
minor refactoring, remove uuid
jrhee17 May 10, 2023
c4a269c
don't whitelist randomuuid'
jrhee17 May 10, 2023
99b9dea
address comments from @ikhoon
jrhee17 May 11, 2023
6cd0f6b
add jvm flag only when needed
jrhee17 May 11, 2023
0998d71
Merge branch 'main' of https://github.com/line/armeria into feature/b…
jrhee17 May 24, 2023
7b6dd68
minor fix due to merge conflict
jrhee17 May 24, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions .github/workflows/actions_build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ jobs:
- java: 19
on: self-hosted
snapshot: true
# blockhound makes the build run about 10 minutes slower
blockhound: true

steps:
- uses: actions/checkout@v2
Expand Down Expand Up @@ -86,6 +88,7 @@ jobs:
${{ (matrix.on == 'self-hosted') && '--max-workers=8' || '--max-workers=2' }} --parallel \
${{ matrix.coverage && '-Pcoverage' || '' }} \
${{ matrix.leak && '-Pleak' || '' }} \
${{ matrix.blockhound && '-Pblockhound' || '' }} \
-PnoLint \
-PflakyTests=false \
-PbuildJdkVersion=${{ env.BUILD_JDK_VERSION }} \
Expand Down Expand Up @@ -133,17 +136,23 @@ jobs:
if: ${{ matrix.coverage }}
uses: codecov/codecov-action@v1

- name: Fail the run if any threads were blocked
if: ${{ matrix.blockhound }}
run: "if [[ -z `find . -name 'blockhound.log' -size +0` ]]; then exit 0; else exit 1; fi"
shell: bash

- name: Collect the test reports
if: failure()
run: find . '(' -name 'java_pid*.hprof' -or -name 'hs_err_*.log' -or -path '*/build/reports/tests' -or -path '*/build/test-results' -or -path '*/javadoc.options' ')' -exec tar rf "reports-JVM-${{ matrix.java }}.tar" {} ';'
run: |
find . '(' -name 'java_pid*.hprof' -or -name 'hs_err_*.log' -or -path '*/build/reports/tests' -or -path '*/build/test-results' -or -path '*/javadoc.options' -or -name 'blockhound.log' ')' -exec tar rf "reports-JVM-${{ matrix.on }}-${{ matrix.java }}.tar" {} ';'
shell: bash

- name: Upload the artifacts
if: failure()
uses: actions/upload-artifact@v2
with:
name: reports-JVM-${{ matrix.java }}
path: reports-JVM-${{ matrix.java }}.tar
name: reports-JVM-${{ matrix.on }}-${{ matrix.java }}
path: reports-JVM-${{ matrix.on }}-${{ matrix.java }}.tar
jrhee17 marked this conversation as resolved.
Show resolved Hide resolved
retention-days: 3

lint:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Copyright 2022 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.common.brave;

import com.linecorp.armeria.common.annotation.UnstableApi;

import reactor.blockhound.BlockHound.Builder;
import reactor.blockhound.integration.BlockHoundIntegration;

/**
* A {@link BlockHoundIntegration} for the brave module.
*/
@UnstableApi
public final class BraveBlockHoundIntegration implements BlockHoundIntegration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jrhee17 jrhee17 May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Then it's fine as it is :-)


@Override
public void applyTo(Builder builder) {
builder.allowBlockingCallsInside("zipkin2.reporter.AsyncReporter$BoundedAsyncReporter", "report");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
com.linecorp.armeria.common.brave.BraveBlockHoundIntegration
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,8 @@
import com.linecorp.armeria.common.brave.HelloService.AsyncIface;
import com.linecorp.armeria.common.brave.RequestContextCurrentTraceContext;
import com.linecorp.armeria.common.thrift.ThriftFuture;
import com.linecorp.armeria.common.util.Exceptions;
import com.linecorp.armeria.common.util.ThreadFactories;
import com.linecorp.armeria.internal.testing.BlockingUtils;
import com.linecorp.armeria.server.AbstractHttpService;
import com.linecorp.armeria.server.HttpService;
import com.linecorp.armeria.server.ServerBuilder;
Expand Down Expand Up @@ -569,17 +569,13 @@ private static class SpanHandlerImpl extends SpanHandler {

@Override
public boolean end(TraceContext context, MutableSpan span, Cause cause) {
return spans.add(span);
return BlockingUtils.blockingRun(() -> spans.add(span));
}

MutableSpan[] take(int numSpans) {
final List<MutableSpan> taken = new ArrayList<>();
while (taken.size() < numSpans) {
try {
taken.add(spans.poll(30, TimeUnit.SECONDS));
} catch (InterruptedException e) {
return Exceptions.throwUnsafely(e);
}
BlockingUtils.blockingRun(() -> taken.add(spans.poll(30, TimeUnit.SECONDS)));
}

// Reverse the collected spans to sort the spans by request time.
Expand Down
15 changes: 15 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,17 @@ allprojects {
systemProperty "java.security.manager", "allow"
}

// required by blockhound for jvm 13+. See https://github.com/reactor/BlockHound/issues/33.
if (rootProject.ext.testJavaVersion >= 13) {
jvmArgs "-XX:+AllowRedefinitionToAddDeleteMethods"
}

// Use verbose exception/response reporting for easier debugging.
systemProperty 'com.linecorp.armeria.verboseExceptions', 'true'
systemProperty 'com.linecorp.armeria.verboseResponses', 'true'

systemProperty 'com.linecorp.armeria.blockhound.reportFile', "${project.buildDir}/blockhound.log"

// Pass special system property to tell our tests that we are measuring coverage.
if (project.hasFlags('coverage')) {
systemProperty 'com.linecorp.armeria.testing.coverage', 'true'
Expand Down Expand Up @@ -165,6 +172,9 @@ configure(projectsWithFlags('java')) {
// Reflections
implementation libs.reflections

// Blockhound
optionalImplementation libs.blockhound

// Test-time dependencies
testImplementation libs.guava.testlib
testImplementation libs.junit4
Expand All @@ -183,6 +193,11 @@ configure(projectsWithFlags('java')) {
testImplementation libs.apache.httpclient5
testImplementation libs.hamcrest
testImplementation libs.hamcrest.library
testRuntimeOnly libs.kotlin.coroutines.debug

if (rootProject.hasProperty('blockhound')) {
testRuntimeOnly libs.blockhound.junit.platform
}
}

// Configure the default DuplicatesStrategy for such as:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import com.google.common.annotations.VisibleForTesting;

import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

import io.netty.channel.EventLoop;

abstract class AbstractEventLoopState {
Expand All @@ -35,7 +37,7 @@ static AbstractEventLoopState of(List<EventLoop> eventLoops, int maxNumEventLoop
return new HeapBasedEventLoopState(eventLoops, maxNumEventLoops, scheduler);
}

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();
private final List<EventLoop> eventLoops;
private final DefaultEventLoopScheduler scheduler;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.ReleasableHolder;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

import io.netty.channel.EventLoop;
import io.netty.channel.EventLoopGroup;
Expand All @@ -58,7 +59,7 @@ final class DefaultEventLoopScheduler implements EventLoopScheduler {

static final int DEFAULT_MAX_NUM_EVENT_LOOPS = 1;

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();

private final List<EventLoop> eventLoops;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import com.linecorp.armeria.common.Cookies;
import com.linecorp.armeria.common.Scheme;
import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

import io.netty.util.NetUtil;
import it.unimi.dsi.fastutil.objects.Object2LongOpenHashMap;
Expand All @@ -60,7 +61,7 @@ final class DefaultCookieJar implements CookieJar {
this.cookiePolicy = cookiePolicy;
store = new Object2LongOpenHashMap<>();
filter = new HashMap<>();
lock = new ReentrantLock();
lock = new ReentrantShortLock();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import java.util.concurrent.TimeoutException;
import java.util.concurrent.atomic.AtomicReference;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;

import com.google.common.base.MoreObjects;
import com.google.common.base.MoreObjects.ToStringHelper;
Expand All @@ -45,6 +44,7 @@
import com.linecorp.armeria.common.util.AsyncCloseableSupport;
import com.linecorp.armeria.common.util.EventLoopCheckingFuture;
import com.linecorp.armeria.common.util.ListenableAsyncCloseable;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

/**
* A dynamic {@link EndpointGroup}. The list of {@link Endpoint}s can be updated dynamically.
Expand All @@ -66,7 +66,7 @@ public static DynamicEndpointGroupBuilder builder() {
private final EndpointSelectionStrategy selectionStrategy;
private final AtomicReference<EndpointSelector> selector = new AtomicReference<>();
private volatile List<Endpoint> endpoints = UNINITIALIZED_ENDPOINTS;
private final Lock endpointsLock = new ReentrantLock();
private final Lock endpointsLock = new ReentrantShortLock();

private final CompletableFuture<List<Endpoint>> initialEndpointsFuture = new InitialEndpointsFuture();
private final AsyncCloseableSupport closeable = AsyncCloseableSupport.of(this::closeAsync);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.common.base.MoreObjects;

import com.linecorp.armeria.client.endpoint.FileWatcherRunnable.FileWatchEvent;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

/**
* A registry which wraps a {@link WatchService} and allows paths to be registered.
Expand Down Expand Up @@ -133,7 +134,7 @@ void close() throws IOException {

private final Map<FileSystem, FileSystemWatchContext> fileSystemWatchServiceMap =
new HashMap<>();
private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();

/**
* Registers a {@code filePath} and {@code callback} to the {@link WatchService}. When the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,14 @@
import java.util.function.Supplier;

import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

/**
* A restartable thread utility class.
*/
final class RestartableThread {

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();

@Nullable
private Thread thread;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import com.linecorp.armeria.client.Endpoint;
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

/**
* This selector selects an {@link Endpoint} using random and the weight of the {@link Endpoint}. If there are
Expand All @@ -37,7 +38,7 @@
*/
final class WeightedRandomDistributionEndpointSelector {

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();
private final List<Entry> allEntries;
@GuardedBy("lock")
private final List<Entry> currentEntries;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.util.AsyncCloseable;
import com.linecorp.armeria.common.util.EventLoopCheckingFuture;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

import io.netty.channel.EventLoopGroup;
import io.netty.util.concurrent.Future;
Expand All @@ -54,7 +55,7 @@ final class DefaultHealthCheckerContext
private final Endpoint endpoint;
private final SessionProtocol protocol;
private final ClientOptions clientOptions;
private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();

/**
* Keeps the {@link Future}s which were scheduled via this {@link ScheduledExecutorService}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.linecorp.armeria.common.annotation.Nullable;
import com.linecorp.armeria.common.metric.MeterIdPrefix;
import com.linecorp.armeria.common.util.AsyncCloseable;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;

import io.micrometer.core.instrument.binder.MeterBinder;

Expand Down Expand Up @@ -110,7 +111,7 @@ public static HealthCheckedEndpointGroupBuilder builder(EndpointGroup delegate,
@VisibleForTesting
final HealthCheckStrategy healthCheckStrategy;

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();
@GuardedBy("lock")
private final Deque<HealthCheckContextGroup> contextGroupChain = new ArrayDeque<>(4);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import com.linecorp.armeria.common.util.AsyncCloseable;
import com.linecorp.armeria.common.util.AsyncCloseableSupport;
import com.linecorp.armeria.common.util.TimeoutMode;
import com.linecorp.armeria.internal.common.util.ReentrantShortLock;
import com.linecorp.armeria.unsafe.PooledObjects;

import io.netty.util.AsciiString;
Expand All @@ -60,7 +61,7 @@ final class HttpHealthChecker implements AsyncCloseable {

private static final AsciiString ARMERIA_LPHC = HttpHeaderNames.of("armeria-lphc");

private final ReentrantLock lock = new ReentrantLock();
private final ReentrantLock lock = new ReentrantShortLock();
private final HealthCheckerContext ctx;
private final WebClient webClient;
private final String authority;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/*
* Copyright 2023 LINE Corporation
*
* LINE Corporation licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

package com.linecorp.armeria.common;

import java.util.ResourceBundle;

import com.linecorp.armeria.common.annotation.UnstableApi;

import reactor.blockhound.BlockHound.Builder;
import reactor.blockhound.integration.BlockHoundIntegration;

/**
* A {@link BlockHoundIntegration} for the core module.
*/
@UnstableApi
public final class CoreBlockHoundIntegration implements BlockHoundIntegration {
@Override
public void applyTo(Builder builder) {
// short locks
builder.allowBlockingCallsInside("com.linecorp.armeria.client.HttpClientFactory",
"pool");
builder.allowBlockingCallsInside("com.linecorp.armeria.internal.common.util.ReentrantShortLock",
"lock");

// Thread.yield can be eventually called when PooledObjects.copyAndClose is called
builder.allowBlockingCallsInside("io.netty.util.internal.ReferenceCountUpdater", "release");

// hdr histogram holds locks
builder.allowBlockingCallsInside("org.HdrHistogram.ConcurrentHistogram", "getCountAtIndex");
builder.allowBlockingCallsInside("org.HdrHistogram.WriterReaderPhaser", "flipPhase");

// StreamMessageInputStream internally uses a blocking queue
// ThreadPoolExecutor.execute internally uses a blocking queue
builder.allowBlockingCallsInside("java.util.concurrent.LinkedBlockingQueue", "offer");

// a single blocking call is incurred for the first invocation, but the result is cached.
builder.allowBlockingCallsInside("com.linecorp.armeria.internal.client.PublicSuffix",
"get");
builder.allowBlockingCallsInside("java.util.ServiceLoader$LazyClassPathLookupIterator",
"parse");
builder.allowBlockingCallsInside(ResourceBundle.class.getName(), "getBundle");
builder.allowBlockingCallsInside("io.netty.handler.codec.compression.Brotli", "<clinit>");

// a lock is held temporarily when adding workers
builder.allowBlockingCallsInside("java.util.concurrent.ThreadPoolExecutor", "addWorker");

// prometheus exporting holds a lock temporarily
builder.allowBlockingCallsInside(
"com.linecorp.armeria.server.metric.PrometheusExpositionService", "doGet");

// Thread.yield can be called
builder.allowBlockingCallsInside(
"java.util.concurrent.FutureTask", "handlePossibleCancellationInterrupt");
}
}
Loading
Loading