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

Whitelist standard streams (System.out/System.err) #125

Merged
merged 1 commit into from May 27, 2020

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented May 27, 2020

No description provided.

@bsideup bsideup added the type/enhancement A general enhancement label May 27, 2020
@bsideup bsideup added this to the next milestone May 27, 2020
@bsideup bsideup mentioned this pull request May 27, 2020
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.

small doc nitpick: it should probably be documented that installing blockhound with this integration wraps the stdout/stderr (in case somebody uses their own wrapper and expects System.out to be of a certain class, for some reason). and also that this integration is rendered moot if the stdout/stderr are set after the integration is installed.

@simonbasle
Copy link
Member

@bsideup does it really fix the logging use case (see #120) ? I mean, if there is no console appender, but rather eg. an async-to-file appender?

@bsideup
Copy link
Contributor Author

bsideup commented May 27, 2020

@simonbasle if it is async-to-file then the expectation that it won't block. If it does - it is rather a problem that the user should know about.

The change solves a very common case of something that should be whitelisted for 99.(9)% of users, because doing System.out.println() from async code is common (especially for debugging))

@simonbasle
Copy link
Member

@bsideup ok then! my comment about javadoc still stands, but other than that 👍

@bsideup
Copy link
Contributor Author

bsideup commented May 27, 2020

it should probably be documented that installing blockhound with this integration wraps the stdout/stderr (in case somebody uses their own wrapper and expects System.out to be of a certain class, for some reason).
and also that this integration is rendered moot if the stdout/stderr are set after the integration is installed.

I think both these points are kinda the expectations you get from System.out/System.setOut API.

  1. You should never expect it (System.out value) to be the same (or of certain class) because it is global. Gradle, for example, overrides it too.
  2. If the user decides to replace the value (either manually or my using a framework that does that), the overridden value is unknown and will either use the previous System.out value (because that's the only reference to the stdout of your process unless you use native code), or redirect it to some completely different destination where new rules apply

@bsideup bsideup merged commit 63bfb5b into master May 27, 2020
@bsideup bsideup deleted the handle_standard_streams branch May 27, 2020 16:23
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.

None yet

2 participants