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

Conversation

simonbasle
Copy link
Member

@simonbasle simonbasle commented Mar 25, 2021

This commit introduces a STATIC_INITIALIZER constant in BlockHound, so
it becomes easier to discover that the features relying on the
String methodName parameter can work with <clinit> for static
initializer.

Additionally, this is documented in the customization examples and
used in the StaticInitTest.

Fixes #178.

This commit introduces a STATIC_INITIALIZER constant in BlockHound, so
it becomes easier to discover that the features relying on the
`String methodName` parameter can work with `<clinit>` for static
initializer.

Additionally, this is documented in the customization examples and
used in the StaticInitTest.

Fixes #178.
@simonbasle simonbasle force-pushed the 176-allowInStaticInitDiscoverability branch from 23817ed to d635dbf Compare March 25, 2021 09:59
@simonbasle
Copy link
Member Author

(oops, the branch is misnamed and should refer to #178 rather than 176)

@simonbasle simonbasle self-assigned this Mar 25, 2021
@simonbasle simonbasle requested a review from a team March 25, 2021 10:01
@simonbasle
Copy link
Member Author

cc @rstoyanchev @bsideup

Copy link

@rstoyanchev rstoyanchev left a comment

Choose a reason for hiding this comment

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

I like the idea but could you please also add a paragraph in the Javadoc of allow and disallow that explicitly mentions this scenario?:

<p>To allow blocking in the static initializer block of a class, use the constant `{@link #STATIC_INITIALIZER} as the method name`.

@simonbasle
Copy link
Member Author

I like the idea but could you please also add a paragraph in the Javadoc of allow and disallow that explicitly mentions this scenario?

Done 👍

* 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

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?

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

@simonbasle simonbasle added the type/enhancement A general enhancement label Mar 25, 2021
@simonbasle
Copy link
Member Author

@rstoyanchev @bsideup dc9186b introduces a BuilderAllowSpec. Since we only support static init for now, maybe it is a bit overkill. It would be even worse if we wanted to prevent going back to the Builder (via and()) without having configured at least one method first (as this would mean introducing 2 interfaces for the builder, so that .allowBlockingCallsInside("foo").and() isn't possible)

agent/src/main/java/reactor/blockhound/BlockHound.java Outdated Show resolved Hide resolved
* {@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.

@simonbasle simonbasle changed the title Add constant STATIC_INITIALIZER for allowBlockingCalls fix #178 by introducing a BuilderAllowSpec intermediate step in the builder Mar 29, 2021
Copy link
Contributor

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

I do not support the idea of adding DSL just for the sake of marking static constructors.
Also, disallowing should be supported.

Last but not least, it seems that static constructor is the only type of methods added by it, which only confirms that a dedicated DSL is an overkill

@simonbasle simonbasle closed this Mar 29, 2021
@simonbasle simonbasle deleted the 176-allowInStaticInitDiscoverability branch March 29, 2021 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide API/documentation on how to allow/disallow blocking calls inside static initializer
3 participants