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

Clarify how to use processExistingClasses in debugging.adoc #3130

Merged
merged 2 commits into from Aug 1, 2022

Conversation

kicmeri
Copy link
Contributor

@kicmeri kicmeri commented Jul 27, 2022

Proposing the change as I was unable to make it work in test method or with @BeforeAll. Hoping it may save time for others.

@kicmeri kicmeri requested a review from a team as a code owner July 27, 2022 09:24
@pivotal-cla
Copy link

@kicmeri Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

1 similar comment
@pivotal-cla
Copy link

@kicmeri Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@kicmeri Thank you for signing the Contributor License Agreement!

@simonbasle
Copy link
Member

@kicmeri can you clarify what didn't work for you and what kind of error you got?

processExistingClasses() is supposed to help with instrumenting classes that have already been loaded, but IIRC not all JVMs support it.

the fact that you had to use a static { } initializer block migh hint that it works thanks to setting up the instrumentation earlier rather than thanks to the processExistingClasses() 🤔

@simonbasle simonbasle changed the title Update debugging.adoc Clarify how to use processExistingClasses in debugging.adoc Jul 27, 2022
@kicmeri
Copy link
Contributor Author

kicmeri commented Jul 27, 2022

@simonbasle I was trying to follow the documentation of reactor.
What I tried with JUnit5 in IntelliJIdea:

    @Test
    public void should_prodDebug() {
        ReactorDebugAgent.init();
        ReactorDebugAgent.processExistingClasses();

        var source = Flux.range(1, 5)
                .single()
                .log();

        StepVerifier.create(source)
                .verifyError();
    }

This produced the exception, but without traceback as I'd have expected - just stacktrace. The introspection still seemed to have been applied (when run in debug mode).

14:43:54.618 [main] ERROR reactor.Mono.Single.1 - onError(java.lang.IndexOutOfBoundsException: Source emitted more than one item)
14:43:54.622 [main] ERROR reactor.Mono.Single.1 - 
java.lang.IndexOutOfBoundsException: Source emitted more than one item
	at reactor.core.publisher.MonoSingle$SingleSubscriber.onNext(MonoSingle.java:134)
...

I also tried with

@BeforeAll
static void setup() {
      ReactorDebugAgent.init();
      ReactorDebugAgent.processExistingClasses();
}

but with same result.

Then I came about ReactorDebugAgentTest that uses the static initializer block - only with that I've seen the traceback in unit test with ReactorDebugAgent.

14:47:10.551 [main] ERROR reactor.Mono.OnAssembly.1 - | onError(java.lang.IndexOutOfBoundsException: Source emitted more than one item)
14:47:10.554 [main] ERROR reactor.Mono.OnAssembly.1 - 
java.lang.IndexOutOfBoundsException: Source emitted more than one item
	at reactor.core.publisher.MonoSingle$SingleSubscriber.onNext(MonoSingle.java:134)
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Assembly trace from producer [reactor.core.publisher.MonoSingle] :
	reactor.core.publisher.Flux.single
	SampleTest.should_prodDebug(SampleTest.java:613)
Error has been observed at the following site(s):
	*__Flux.single ⇢ at SampleTest.should_prodDebug(SampleTest.java:613)
Original Stack Trace:
		at reactor.core.publisher.MonoSingle$SingleSubscriber.onNext(MonoSingle.java:134)
...

Still, it can be that I'm just doing something wrong.

the fact that you had to use a static { } initializer block migh hint that it works thanks to setting up the instrumentation earlier rather than thanks to the processExistingClasses() thinking

I'm not sure what else could have set it up, I'm not using the maven plugin.

Regarding JVM I use

mvn -version
Apache Maven 3.8.5 (3599d3414f046de2324203b78ddcf9b5e4388aa0)
Maven home: /usr/share/apache-maven-3.8.5
Java version: 18.0.2, vendor: Azul Systems, Inc., runtime: /usr/lib/jvm/zulu18-ca-amd64
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "5.4.0-113-generic", arch: "amd64", family: "unix"

@simonbasle
Copy link
Member

simonbasle commented Jul 27, 2022

thanks for the clarifications, that's good feedback.
I wonder if it would work with a TestExecutionListener 🤔

note that this section of the documentation isn't intended as a recipe for unit tests specifically, but more as an indicator that the tool offers a path to JVM's Instrumentation.retransformClasses where it makes sense

@kicmeri
Copy link
Contributor Author

kicmeri commented Jul 27, 2022

@simonbasle The setup with TestExecutionListener works for me too producing the traceback.

Nonetheless, all I intended is to improve the doc. I see it is questionable whether it is really an improvement. Thus, when you say

the documentation isn't intended as a recipe for unit tests specifically

feel free to close this PR.

Copy link
Member

@simonbasle simonbasle left a comment

Choose a reason for hiding this comment

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

I think even though this wasn't intended as an example, it did confuse you.

We can probably add the TestExecutionListener example. The sentence before the snippet would need a bit of splitting and rewording. What do you think about:

You may also re-process existing classes with `processExistingClasses()` if you cannot run the init eagerly.
For example, in a JUnit5 `TestExecutionListener`:

then the modified sample (not static block but listener)

@kicmeri
Copy link
Contributor Author

kicmeri commented Jul 28, 2022

@simonbasle Well, thinking about it: My purpose was not exactly to use the PROD debugging feature in test. I just tried it in the test. When I do it from regular application in my setup

public class ProdDebugApp {
    public static void main(String... args) {
        ReactorDebugAgent.init();
        ReactorDebugAgent.processExistingClasses();

        var source = Flux.range(1, 5)
                .single()
                .log();

        StepVerifier.create(source)
                .verifyError();
    }
}

I still get the no traceback. with the above code it again works only with static initializer, no JUnit framework involved at all.

Thus the wording pointing to unit testing is rather misleading and I'd prefer some hint on what to try, when the misbehaviour happens, regardless of the cause.

@simonbasle simonbasle added the type/documentation A documentation update label Jul 28, 2022
@simonbasle simonbasle added this to the 3.5.0-M5 milestone Jul 28, 2022
@simonbasle
Copy link
Member

yeah I think that works 👍

Proposing the change as I was unable to make it work in test method or with `@BeforeAll`. Hoping it may save time for others.
@simonbasle
Copy link
Member

I'll rebase this PR on top of 3.4.x, which should also trigger a new run of CI, then will merge 👍

@simonbasle simonbasle changed the base branch from main to 3.4.x August 1, 2022 08:27
@simonbasle simonbasle modified the milestones: 3.5.0-M5, 3.4.22 Aug 1, 2022
@simonbasle simonbasle merged commit 41e956d into reactor:3.4.x Aug 1, 2022
@reactorbot
Copy link

@simonbasle this PR seems to have been merged on a maintenance branch, please ensure the change is merge-forwarded to intermediate maintenance branches and up to main 🙇

simonbasle added a commit that referenced this pull request Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/documentation A documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants