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

fix #178 by introducing a BuilderAllowSpec intermediate step in the builder #179

Closed
wants to merge 7 commits into from
75 changes: 74 additions & 1 deletion agent/src/main/java/reactor/blockhound/BlockHound.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
*/
public class BlockHound {

static final String PREFIX = "$$BlockHound$$_";
static final String PREFIX = "$$BlockHound$$_";

private static final AtomicBoolean INITIALIZED = new AtomicBoolean(false);

Expand Down Expand Up @@ -120,6 +120,10 @@ public TypePool typePool(ClassFileLocator classFileLocator, ClassLoader classLoa
}
}

/**
* A builder for configuration of the BlockHound java agent, passed to each {@link BlockHoundIntegration}'s
* {@link BlockHoundIntegration#applyTo(Builder) applyTo} method when {@link BlockHound#install(BlockHoundIntegration...) installing}.
*/
public static class Builder {

private final Map<String, Map<String, Set<String>>> blockingMethods = new HashMap<String, Map<String, Set<String>>>() {{
Expand Down Expand Up @@ -254,6 +258,58 @@ public static class Builder {

private Predicate<Thread> dynamicThreadPredicate = t -> false;

/**
* A phase of the builder where allowed blocking methods are configured for a given class.
* See {@link Builder#allowBlockingCallsInside(String)}.
* <p>
* Note that methods of this class directly mutate the {@link Builder} as if one was calling
* {@link Builder#allowBlockingCallsInside(String, String)}. One MUST specify at least one
* allowed method for it to have an effect on the configuration.
*/
public class BuilderAllowSpec {

Choose a reason for hiding this comment

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

I don't see anything for "Disallow". Doesn't that need the same? The same builder could support both.

Copy link
Member Author

Choose a reason for hiding this comment

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

it could, but I wasn't able to test the disallow feature. Either I misunderstand it or there is something wrong with the feature (which isn't tested)... so for now I prefer to focus on the allow-static-initializer case which is demonstrably supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I saw that and didn't find it crystal clear.
The following tests fail:

public class BlockingDisallowTest {

    static {
        BlockHound.install(b -> b
                .disallowBlockingCallsInside("reactor.core.publisher.Flux", "just")
                .disallowBlockingCallsInside(NonBlockingClass.class.getName(), "example")
        );
    }

    @Test
    public void shouldDisallowFluxJust() {
        assertThatExceptionOfType(RuntimeException.class)
                .isThrownBy(() ->
                        Mono.just("one")
                            .publishOn(Schedulers.parallel())
                            .flatMapMany(i -> Flux.just("one", "two"))
                            .blockLast()
                )
                .withCauseInstanceOf(BlockingOperationError.class);

    }

    @Test
    public void shouldDisallowNonBlocking() {
        NonBlockingClass nbc = new NonBlockingClass();

        assertThatExceptionOfType(RuntimeException.class)
                .isThrownBy(() ->
                        Mono.fromCallable(nbc::example)
                            .subscribeOn(Schedulers.parallel())
                            .block()
                )
                .withCauseInstanceOf(BlockingOperationError.class);
    }

    static class NonBlockingClass {

        String example() {
            return "example";
        }
    }
}

What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to be missing how BlockHound works, see https://github.com/reactor/BlockHound/blob/master/docs/how_it_works.md and especially "Blocking call decision".

tl;dr: disallowBlockingCallsInside disallows blocking calls inside provided method (as per the API method's name). It does not, however, mark the method as blocking, so your Flux.just or NonBlockingClass.example makes no sense.


final String className;

private BuilderAllowSpec(String className) {
this.className = className;
}

/**
* Allow blocking calls inside the static initializer block of the currently configured
* class. Use {@link #and()} to exit the class-specific phase and return
* to the {@link Builder}.
*
* @return this {@link BuilderAllowSpec} for further configuration of the given class
* @see #and()
*/
public BuilderAllowSpec forStaticInitializer() {
Builder.this.allowBlockingCallsInside(this.className, "<clinit>");
return this;
}
simonbasle marked this conversation as resolved.
Show resolved Hide resolved

/**
* Allow blocking calls inside the specified method of the currently configured
* class. Use {@link #and()} to exit the class-specific phase and return
* to the {@link Builder}.
*
* @return this {@link BuilderAllowSpec} for further configuration of the given class
* @see #and()
*/
public BuilderAllowSpec forMethod(String methodName) {
Builder.this.allowBlockingCallsInside(this.className, methodName);
return this;
}
simonbasle marked this conversation as resolved.
Show resolved Hide resolved

/**
* Exit the class-specific phase and return to the {@link Builder}.
*
* @return the {@link Builder}
*/
public Builder and() {
return Builder.this;
}
}
simonbasle marked this conversation as resolved.
Show resolved Hide resolved

/**
* Marks provided method of the provided class as "blocking".
*
Expand Down Expand Up @@ -290,16 +346,33 @@ public Builder markAsBlocking(String className, String methodName, String signat
/**
* Allows blocking calls inside any method of a class with name identified by the provided className
* and which name matches the provided methodName.
* <p>
* A sub-builder is available to guide users towards more advanced cases like static initializers,
* see {@link #allowBlockingCallsInside(String)} and {@link BuilderAllowSpec}.
*
* @param className class' name (e.g. "java.lang.Thread")
* @param methodName a method name
* @return this
* @see #allowBlockingCallsInside(String)
* @see BuilderAllowSpec
*/
public Builder allowBlockingCallsInside(String className, String methodName) {
allowances.computeIfAbsent(className, __ -> new HashMap<>()).put(methodName, true);
return this;
}

/**
* Configure allowed blocking calls inside methods of a class with name identified by the provided className.
* Note that this doesn't change the configuration unless one further specifies which methods are allowed.
* Use {@link BuilderAllowSpec#and()} to get back to this {@link Builder}.
*
* @param className class' name (e.g. "java.lang.Thread")
* @return a sub-step of the builder as a {@link BuilderAllowSpec}
*/
public BuilderAllowSpec allowBlockingCallsInside(String className) {
return new BuilderAllowSpec(className);
}

/**
* Disallows blocking calls inside any method of a class with name identified by the provided className
* and which name matches the provided methodName.
Expand Down
8 changes: 8 additions & 0 deletions docs/customization.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,14 @@ builder.disallowBlockingCallsInside(
);
```

This will allow blocking method calls inside the static initializer block of `MyClass` down the callstack:
```java
builder.allowBlockingCallsInside(
"com.example.MyClass",
BlockHound.STATIC_INITIALIZER
);
```

## Custom blocking method callback
* `Builder#blockingMethodCallback(Consumer<BlockingMethod> consumer)`

Expand Down
113 changes: 113 additions & 0 deletions example/src/test/java/com/example/BlockingAllowSpecTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright (c) 2020-Present Pivotal Software Inc, All Rights Reserved.
*
* Licensed 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.example;

import org.assertj.core.api.Assertions;
import org.junit.Test;

import reactor.blockhound.BlockHound;
import reactor.blockhound.BlockingOperationError;
import reactor.core.publisher.Mono;
import reactor.core.scheduler.Schedulers;

public class BlockingAllowSpecTest {

static {
BlockHound.install(b -> b
.allowBlockingCallsInside(BlockingClassA.class.getName())
.forStaticInitializer()
.forMethod("block")
.and()
.allowBlockingCallsInside(BlockingClassB.class.getName())
.forMethod("block1")
.forMethod("block2")
);
}

@Test
public void shouldInstrumentBlockingClassA() {
Mono.fromCallable(BlockingClassA::new)
.map(BlockingClassA::block)
.subscribeOn(Schedulers.parallel())
.block();
}

@Test
public void shouldInstrumentBlockingClassB() {
Mono.fromCallable(() -> {
BlockingClassB b = new BlockingClassB();
b.block1();
b.block2();
return b;
}).subscribeOn(Schedulers.parallel()).block();
}

@Test
public void usingAllowSpecWithoutCallingMethodIsIgnored() {
//getting the BuilderAllowSpec and not acting on it DOESN'T equate to allowing any method: it does nothing
BlockHound.install(b -> b.allowBlockingCallsInside(BlockingClassC.class.getName()));

Mono<String> mono = Mono.fromCallable(BlockingClassC::new)
.publishOn(Schedulers.parallel())
.map(BlockingClassC::block1);

Assertions.assertThatExceptionOfType(RuntimeException.class)
.isThrownBy(mono::block)
.havingCause()
.isInstanceOf(BlockingOperationError.class)
.withMessage("Blocking call! java.lang.Thread.yield")
.withStackTraceContaining("at com.example.BlockingAllowSpecTest$BlockingClassC.block1");
}

static class BlockingClassA {
static {
try {
Thread.sleep(0);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}

String block() {
Thread.yield();
return "hello A";
}
}

static class BlockingClassB {

void block1() {
try {
Thread.sleep(0);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}

void block2() {
Thread.yield();
}
}

static class BlockingClassC {

String block1() {
Thread.yield();
return "hello C";
}
}
}
5 changes: 2 additions & 3 deletions example/src/test/java/com/example/StaticInitTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,15 @@
package com.example;

import org.junit.Test;

import reactor.blockhound.BlockHound;
import reactor.core.publisher.Mono;
import reactor.core.scheduler.Schedulers;

public class StaticInitTest {

static {
BlockHound.install(b -> {
b.allowBlockingCallsInside(ClassWithStaticInit.class.getName(), "<clinit>");
});
BlockHound.install(b -> b.allowBlockingCallsInside(ClassWithStaticInit.class.getName()).forStaticInitializer());
}

@Test
Expand Down