(WIP) BlockHound integration #1873
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
qwwdfsad
reviewed
Mar 27, 2020
There was a problem hiding this comment.
Changes look good.
Let's wait for ByteBuddy 1.10.9 (where raphw/byte-buddy#825 is fixed) and merge it.
|
Also, please rebase current branch on top of our develop |
dkhalanskyjb
force-pushed
the
blockHound
branch
from
e76f10b to
5b7621c
Compare
|
Sure. I was on it the moment I saw the publication validator fix merged. |
This way, it can work even with BlockHound on JDK8, which also uses ByteBuddy and thus was in conflict. Kind of solves #1060, but since now the debugging routine depends on BlockHound, where, it seems, the same problem was not fixed, the original cause for concern probably still stands.
Integration with BlockHound revealed that several tests were performing blocking operations such as Thread.sleep in coroutine context `Dispatchers.DEFAULT`. Also, some tests hanged because the exception that notified about blocking calls were wildcard-matched.
Before, the tests only knew that the `park` native method was moved to `jdk.internal.misc.Unsafe` from `sun.misc.Unsafe`. However, in JDK 11, there is no `sun.misc.Unsafe` at all, it seems that it has been moved completely. This change is beneficial for this feature set because BlockHound puts its `$$BlockHound$$_park` method to `sun.misc.Unsafe` or to `jdk.internal.misc.Unsafe` depending on the JDK version. Also, judging by BlockHound's code https://github.com/reactor/BlockHound/blob/091d7b139479b1c41eea59baa23389d673fdf73b/agent/src/main/java/reactor/blockhound/BlockHound.java#L177-L187, `forkAndExec` had been moved, too, but it is not used in tests.
Now it relies on the ServiceLoader mechanism to load the integration: if the user installs BlockHound and `kotlinx-coroutines-debug` is in the classpath, then BlockHound will know to detect blocking calls in scenarios that forbid it.
The feature that we need was released, so no need to incclude the snapshot repositories in the build.
* publication-validator is renamed to integration-testing; * Each test is now in a separate source set, which allows for more flexibility in their configuration; for example, failing to set `dryRun=true` doesn't prevent tests other than NPM to run, and it is possible to run the tests (and their dependencies) separately.
Co-Authored-By: Sergei Egorov <[email protected]>
The `shadow` configuration extends `api` now, which means that all the `api`-style dependencies of this module are not bundled in the fat jar and are instead mentioned in the POM file as `RUNTIME` dependencies. This means that now JNA is not included with the jar, but when someone depends on the jar to later call `DebugProbes.install()`, JNA is fetched as a transitive dependency. This is what we want: JNA is not needed in the jar, because when the debugger is run as a Java agent, attachment of Byte Buddy happens in `Installer`, and JNA-based attach emulation is not used; Byte Buddy is also written is such a way that if JNA is not in the class path, nothing breaks unless one tries to use attach emulation.
Merged
qwwdfsad
added a commit
that referenced
this pull request
Apr 24, 2020
* Integration with BlockHound * Improve build configuration of integration tests * publication-validator is renamed to integration-testing; * Add an integration test for coroutine debugger java agent * Use JNA-based attach mechanism for dynamic attach Fixes #1821 Fixes #1060 Co-authored-by: Vsevolod Tolstopyatov <[email protected]> Co-authored-by: Sergei Egorov <[email protected]>
recheej
pushed a commit
to recheej/kotlinx.coroutines
that referenced
this pull request
Dec 28, 2020
* Integration with BlockHound * Improve build configuration of integration tests * publication-validator is renamed to integration-testing; * Add an integration test for coroutine debugger java agent * Use JNA-based attach mechanism for dynamic attach Fixes Kotlin#1821 Fixes Kotlin#1060 Co-authored-by: Vsevolod Tolstopyatov <[email protected]> Co-authored-by: Sergei Egorov <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The previous attempt was borked by two things:
kotlinx-coroutines-debug,Now, the first of the two things is fixed. The pull request is marked as WIP because the second one is not.