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

Add BlockHound integration #9687

Merged
merged 1 commit into from
Oct 25, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@
<artifactId>log4j-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>io.projectreactor.tools</groupId>
<artifactId>blockhound</artifactId>
<optional>true</optional>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-core</artifactId>
Expand Down
87 changes: 87 additions & 0 deletions common/src/main/java/io/netty/util/internal/Hidden.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright 2019 The Netty Project
*
* The Netty Project 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:
*
* http://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 io.netty.util.internal;

import io.netty.util.concurrent.FastThreadLocalThread;
import reactor.blockhound.BlockHound;
import reactor.blockhound.integration.BlockHoundIntegration;

import java.util.function.Function;
import java.util.function.Predicate;

/**
* Contains classes that must be have public visibility but are not public API.
*/
class Hidden {

/**
* This class integrates Netty with BlockHound.
* <p>
* It is public but only because of the ServiceLoader's limitations
* and SHOULD NOT be considered a public API.
*/
@UnstableApi
@SuppressJava6Requirement(reason = "BlockHound is Java 8+, but this class is only loaded by it's SPI")
public static final class NettyBlockHoundIntegration implements BlockHoundIntegration {

@Override
public void applyTo(BlockHound.Builder builder) {
builder.allowBlockingCallsInside(
"io.netty.channel.nio.NioEventLoop",
"handleLoopException"
);

builder.allowBlockingCallsInside(
"io.netty.channel.kqueue.KQueueEventLoop",
"handleLoopException"
);

builder.allowBlockingCallsInside(
"io.netty.channel.epoll.EpollEventLoop",
"handleLoopException"
);

builder.allowBlockingCallsInside(
"io.netty.util.HashedWheelTimer$Worker",
"waitForNextTick"
);

builder.allowBlockingCallsInside(
"io.netty.util.concurrent.SingleThreadEventExecutor",
"confirmShutdown"
);

builder.nonBlockingThreadPredicate(new Function<Predicate<Thread>, Predicate<Thread>>() {
Copy link
Contributor

@johnou johnou Oct 23, 2019

Choose a reason for hiding this comment

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

builder.allowBlockingCallsInside(
    "io.netty.channel.nio.NioEventLoop",
    "handleLoopException"
);

builder.allowBlockingCallsInside(
    "io.netty.channel.kqueue.KQueueEventLoop",
    "handleLoopException"
);

builder.allowBlockingCallsInside(
    "io.netty.channel.epoll.EpollEventLoop",
    "handleLoopException"
);

builder.allowBlockingCallsInside(
    "io.netty.channel.socket.oio.OioSocketChannel",
    "checkInputShutdown"
);

builder.allowBlockingCallsInside(
    "io.netty.util.HashedWheelTimer$Worker",
    "waitForNextTick"
);

builder.allowBlockingCallsInside(
    "io.netty.util.concurrent.SingleThreadEventExecutor",
    "confirmShutdown"
);

unsure about ThreadDeathWatcher as it's marked deprecated and not used in the Netty codebase.

Copy link
Member

Choose a reason for hiding this comment

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

@bsideup I wonder if we could just whitelist all of io.netty.* or if there is support to annotate methods with something so these are ignored. Having to specify a string for all of them seems very error-prone

Copy link
Contributor

Choose a reason for hiding this comment

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

@normanmaurer and would probably break with shaded Netty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnou
Shading will work fine because it searches for the string occurrences.
I see that you whitelisted a few util methods. It may not be a good solution, we usually recommend whitelisting the "business logic" methods that call such blocking methods and cannot avoid them. Whitelisting the util method may lead to a situation where it will get called from a method that must not call any blocking methods.

@normanmaurer
See my previous answer. If you whitelist io.netty.*, a user-called Netty code (some util or maybe even internal) will block user's thread but he won't notice.

Copy link
Contributor Author

@bsideup bsideup Oct 23, 2019

Choose a reason for hiding this comment

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

Also... don't you want to test Netty itself with BlockHound to double check that you are not blocking where you should not? 😊

Copy link
Member

Choose a reason for hiding this comment

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

@bsideup I see... I just find using "strings" quite error-prone in practice :/

That said can you just adjust the PR to do the string stuff for now for the methods within netty that use Thread.sleep(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@normanmaurer

FYI since we're talking about Netty's integration and the classes are already loaded, nothing stops you from using:

builder.allowBlockingCallsInside(
    io.netty.util.concurrent.SingleThreadEventExecutor.class.getName(),
    "confirmShutdown"
);

(unless the class is private/internal, in that case, such package may have it's own integration)

I will update the PR and add the whitelisted methods 👍

Anything else you think is missing so far, or this is the only thing? :)

Copy link
Member

Choose a reason for hiding this comment

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

I think that is the only thing... Once this is merged I guess we also want to run in on the CI with BlockHound enabled once a day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, cool, will update the PR in a moment :)

Re the CI - I would even recommend having it on by default for every CI run given the very low overhead, so that the contributions are also protected from the blocking calls.

That's of course up to you, but I would be curious to know the arguments against having it enabled by default. At least we plan to make it the tool for async Java frameworks to verify their asynchronosity ☺️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added all the methods except OioSocketChannel #checkInputShutdown (my IDE shows it as "unused" and the class is deprecated anyways)

@Override
public Predicate<Thread> apply(final Predicate<Thread> p) {
return new Predicate<Thread>() {
@Override
@SuppressJava6Requirement(reason = "Predicate#test")
public boolean test(Thread thread) {
return p.test(thread) || thread instanceof FastThreadLocalThread;
}
};
}
});
}

@Override
bsideup marked this conversation as resolved.
Show resolved Hide resolved
public int compareTo(BlockHoundIntegration o) {
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

Add a proper impl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default method compareTo in BlockHoundIntegration (can't use it due to Java 6 compiler, had to copy) returns 0 by default, because the order of most of integrations does not matter.
And, in Netty, since it does not depend on any other integration, it is so.
Or would you recommend returning some other constant (+/-1)?

Copy link
Member

Choose a reason for hiding this comment

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

maybe just let it return 0 when o == this and -1 otherwise ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to crowdsource it a bit :D
https://twitter.com/bsideup/status/1186220275232321537

Although I see why you're suggesting the 0/-1 option. Just want to double check (and maybe change the default we have in BlockHound)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, after a discussion with Mr. Shipilёv, it seems that 0 is a better choice here, to avoid a situation when a < b and b < a at the same time. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I liked the solution proposed in https://twitter.com/tagir_valeev/status/1186270999383330818 which won't guarantee a consistent ordering over restarts, but is a total ordering for the lifetime of the process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess any non-constant based solution should be done in the default method defined in BlockHound itself, to keep it consistent.
Hereby I suggest leaving 0 here as it is not the responsibility of this integration.
WDYT?

}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Copyright 2019 The Netty Project
#
# The Netty Project 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:
#
# http://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.
io.netty.util.internal.Hidden$NettyBlockHoundIntegration
8 changes: 8 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@
<module>testsuite-osgi</module>
<module>testsuite-shading</module>
<module>testsuite-native-image</module>
<module>transport-blockhound-tests</module>
<module>microbench</module>
<module>bom</module>
</modules>
Expand Down Expand Up @@ -674,6 +675,13 @@
<version>${log4j2.version}</version>
<scope>test</scope>
</dependency>

<!-- BlockHound integration -->
<dependency>
<groupId>io.projectreactor.tools</groupId>
<artifactId>blockhound</artifactId>
<version>1.0.1.RELEASE</version>
</dependency>
</dependencies>
</dependencyManagement>

Expand Down
53 changes: 53 additions & 0 deletions transport-blockhound-tests/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--
~ Copyright 2019 The Netty Project
~
~ The Netty Project 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:
~
~ http://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.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">

<modelVersion>4.0.0</modelVersion>
<parent>
<groupId>io.netty</groupId>
<artifactId>netty-parent</artifactId>
<version>4.1.43.Final-SNAPSHOT</version>
</parent>

<artifactId>netty-transport-blockhound-tests</artifactId>
<packaging>jar</packaging>
<description>
Tests for the BlockHound integration.
</description>

<name>Netty/Transport/BlockHound/Tests</name>

<properties>
<maven.compiler.source>1.8</maven.compiler.source>
<maven.compiler.target>1.8</maven.compiler.target>
<skipJapicmp>true</skipJapicmp>
</properties>

<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>netty-transport</artifactId>
<version>${project.version}</version>
</dependency>

<dependency>
<groupId>io.projectreactor.tools</groupId>
<artifactId>blockhound</artifactId>
<scope>test</scope>
</dependency>
</dependencies>
</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright 2019 The Netty Project

* The Netty Project 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:

* http://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 io.netty.util.internal;

import io.netty.util.concurrent.GlobalEventExecutor;
import org.junit.BeforeClass;
import org.junit.Test;
import reactor.blockhound.BlockHound;

import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
import java.util.concurrent.TimeUnit;

import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

public class NettyBlockHoundIntegrationTest {

@BeforeClass
public static void setUpClass() {
BlockHound.install();
}

@Test
public void testBlockingCallsInNettyThreads() throws Exception {
final FutureTask<Void> future = new FutureTask<>(() -> {
Thread.sleep(0);
return null;
});
GlobalEventExecutor.INSTANCE.execute(future);

try {
future.get(5, TimeUnit.SECONDS);
fail("Expected an exception due to a blocking call but none was thrown");
} catch (ExecutionException e) {
Throwable throwable = e.getCause();
assertNotNull("An exception was thrown", throwable);
assertTrue("Blocking call was reported", throwable.getMessage().contains("Blocking call"));
}
}
}