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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d635dbf
Add constant STATIC_INITIALIZER for allowBlockingCalls
simonbasle Mar 25, 2021
405bb1d
Add mention of constant in the allow/disallow methods javadoc
simonbasle Mar 25, 2021
82d3fb2
Merge branch 'master' into 176-allowInStaticInitDiscoverability
simonbasle Mar 25, 2021
32c596e
Merge branch 'master' into 176-allowInStaticInitDiscoverability
simonbasle Mar 29, 2021
dc9186b
Use a BuilderAllowSpec, as a phase of the builder
simonbasle Mar 29, 2021
d0114bc
Remove and(), spec is single call, vararg forMethods
simonbasle Mar 29, 2021
edbe539
Note that constructors are currently not supported in allowance
simonbasle Mar 29, 2021
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
16 changes: 15 additions & 1 deletion agent/src/main/java/reactor/blockhound/BlockHound.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,13 @@
*/
public class BlockHound {

static final String PREFIX = "$$BlockHound$$_";
/**
* The special methodName that should be used in e.g. {@link Builder#allowBlockingCallsInside(String, String)}
* when one wants to reference the static initializer block rather than a class method.
*/
public static final String STATIC_INITIALIZER = "<clinit>";
Copy link
Contributor

Choose a reason for hiding this comment

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

since this constant would never change (part of the JVM's spec), I do not feel that it makes sense to introduce it. We could keep the docs section (for both <clinit> and <init>), but the constant will only create an unnecessary noise in the public signature of this class.

Copy link
Member Author

@simonbasle simonbasle Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know. Mentioning the reserved method name in the doc indeed feels like a minimum viable solution, but feels slightly less user-friendly than providing a safe constant which can quickly be auto-completed. Would it limit the noise drawback if the constant was defined in Builder instead of BlockHound? @rstoyanchev as a user, what do you think?

Copy link

Choose a reason for hiding this comment

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

I also prefer an explicit API with documentation as a minimum. That said, thinking more about the constant it also gets slightly verbose with the class name included, and it's not automatically discoverable.

What about adding a sub-builder step:

builder
    .allowBlockingCallsInside("com.example.ClassA", "methodA")
    .allowBlockingCallsInside("com.example.ClassB").staticInitializers()
    .disallowBlockingCallsInside("com.example.ClassC").constructors();

The sub-builder something like:

interface BlockingCallSpec {

    BlockHound.Builder staticInitializers();

    BlockHound.Builder constructors();

}

Or perhaps an enum after all?

builder
    .allowBlockingCallsInside("com.example.ClassA", "methodA")
    .allowBlockingCallsInside("com.example.ClassB", BlockHound.TargetType.STATIC_INITIALIZER)
    .disallowBlockingCallsInside("com.example.ClassC", BlockHound.TargetType.CONSTRUCTORS);

Copy link
Member Author

Choose a reason for hiding this comment

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

exploring the builder idea in this PR and the enum in #184


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

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

Expand Down Expand Up @@ -290,10 +296,14 @@ 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>
* Note that for static initializers, you can use {@link BlockHound#STATIC_INITIALIZER} as the methodName.
* Constructors are currently not supported.
*
* @param className class' name (e.g. "java.lang.Thread")
* @param methodName a method name
* @return this
* @see BlockHound#STATIC_INITIALIZER
*/
public Builder allowBlockingCallsInside(String className, String methodName) {
allowances.computeIfAbsent(className, __ -> new HashMap<>()).put(methodName, true);
Expand All @@ -303,10 +313,14 @@ public Builder allowBlockingCallsInside(String className, String methodName) {
/**
* Disallows blocking calls inside any method of a class with name identified by the provided className
* and which name matches the provided methodName.
* <p>
* Note that for static initializers, you can use {@link BlockHound#STATIC_INITIALIZER} as the methodName.
* Constructors are currently not supported.
*
* @param className class' name (e.g. "java.lang.Thread")
* @param methodName a method name
* @return this
* @see BlockHound#STATIC_INITIALIZER
*/
public Builder disallowBlockingCallsInside(String className, String methodName) {
allowances.computeIfAbsent(className, __ -> new HashMap<>()).put(methodName, false);
Expand Down
8 changes: 8 additions & 0 deletions docs/customization.md
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion example/src/test/java/com/example/StaticInitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public class StaticInitTest {

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

Expand Down