Skip to content

BlockHound integration #1821

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

Jump to bottom
Merged
merged 15 commits into from
Mar 16, 2020
Merged

BlockHound integration #1821

merged 15 commits into from
Mar 16, 2020

Conversation

dkhalanskyjb
Copy link
Collaborator

Solves #1031

@dkhalanskyjb
Copy link
Collaborator Author

If everything is good with the general way this is implemented, then some documentation or the other should probably indicate that it's now possible to use BlockHound. Where would be a good place to write about it?

@qwwdfsad qwwdfsad force-pushed the develop branch 3 times, most recently from 4a49830 to aff8202 Compare March 10, 2020 17:27
qwwdfsad
qwwdfsad requested changes Mar 13, 2020
View reviewed changes
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment •
edited
Loading

Choose a reason for hiding this comment

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

then some documentation or the other should probably indicate that it's now possible to use BlockHound

  1. Let's ask @bsideup to mention the fact that we support BH in BH documentation or let's open a pull request to its documentation if he doesn't mind.
  2. Mention BH support in debug module readme and put a link to BH doc there

@dkhalanskyjb
BlockHound integration POC
edc388c
@dkhalanskyjb
Do not shadow ByteBuddy agent
ef33b34 
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.
@dkhalanskyjb
Fix tests not being run with the right dispatcher
69f76dc 
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.
@dkhalanskyjb
Clean the stack traces from BlockHound artifacts
46bbf2a
@dkhalanskyjb
Intermediate value
d1e4798
@dkhalanskyjb
Improve handling of stacktrace tests on JDK 8
5058081 
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.
@dkhalanskyjb
Implement BlockHound integration properly
efb4720 
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.
@dkhalanskyjb
Up the BlockHound version
6e5fe92 
The feature that we need was released, so no need to incclude the
snapshot repositories in the build.
@dkhalanskyjb
Improve build configuration of integration tests
3fc6abc 
* 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.
@dkhalanskyjb
Add an integration test for coroutine debugger java agent
76ab90f
@dkhalanskyjb
Document BlockHound integration better
e16face
@dkhalanskyjb
Do not exclude the ByteBuddy agent from shading
15b54c9
@qwwdfsad qwwdfsad self-requested a review March 16, 2020 13:38
qwwdfsad
qwwdfsad approved these changes Mar 16, 2020
View reviewed changes
Copy link
Collaborator

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Good job!

bsideup
bsideup approved these changes Mar 16, 2020
View reviewed changes
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.

Looks good to me 👍

Added a small comment re the link, but otherwise: :shipit:

@qwwdfsad qwwdfsad merged commit ed7c668 into develop Mar 16, 2020
@qwwdfsad qwwdfsad deleted the blockHound branch March 16, 2020 14:27
qwwdfsad added a commit that referenced this pull request Mar 17, 2020
@qwwdfsad
Revert "BlockHound integration (#1821)"
aa86ff3 
This reverts commit ed7c668.
@elizarov elizarov restored the blockHound branch March 17, 2020 14:37
@cj848
Copy link

cj848 commented Apr 3, 2020 •
edited
Loading

I saw this code and tried it myself in my application, but it doesn't work because of package security. Can you release these changes before 1.4?

qwwdfsad added a commit that referenced this pull request Apr 6, 2020
@dkhalanskyjb @qwwdfsad @bsideup
BlockHound integration (#1873)
02b403d 
* 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]>
@dkhalanskyjb
Copy link
Collaborator Author

I saw this code and tried it myself in my application, but it doesn't work because of package security. Can you release these changes before 1.4?

Another pull request was merged with these changes: #1873.

qwwdfsad added a commit that referenced this pull request Apr 24, 2020
@dkhalanskyjb @qwwdfsad @bsideup
BlockHound integration (#1873)
c1f8834 
* 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
@dkhalanskyjb @qwwdfsad
@bsideup @recheej
BlockHound integration (Kotlin#1873)
e127d69 
* 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
Reviewers

@bsideup bsideup bsideup approved these changes

@qwwdfsad qwwdfsad qwwdfsad approved these changes

Assignees
No one assigned
Labels
For 1.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dkhalanskyjb @cj848 @bsideup @qwwdfsad @elizarov