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

Prevent stray output from reaching stdout #618

Merged
merged 6 commits into from
Apr 28, 2024

Conversation

phiSgr
Copy link
Contributor

@phiSgr phiSgr commented Mar 18, 2023

prevents prints that happen after (user's) main returns from polluting stdout.

Context: https://stackoverflow.com/questions/75714151/kotlin-coroutines-order-of-printing-logs/75760394#75760394

@phiSgr phiSgr changed the title do not switch System.out back Prevent stray output from reaching stdout Mar 21, 2023
@phiSgr
Copy link
Contributor Author

phiSgr commented Mar 21, 2023

On second thought, not switching System.out back may cause stray prints become data races as the main thread reads from ByteArrayOutputStream and the thread the prints writes to it.

But if that is the reason for calling setOut, System.err should also be switched back.


Or if the writes synchronize over the BAOS as I suggested in #619, the data race can be prevented by

synchronized(outputStream) {
    outputObj.text = outputStream.toString()
        .replace("</errStream><errStream>".toRegex(), "")
        .replace("</outStream><outStream>".toRegex(), "")
}

@AlexanderPrendota
Copy link
Collaborator

AlexanderPrendota commented Mar 21, 2023

Hi, is it possible to add a test (just a snippet to run)?

)
Assertions.assertEquals("<outStream>I'm sleeping 0 ...\nI'm sleeping 1 ...\nI'm sleeping 2 ...\n</outStream>", result.text)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you go before the fix with git checkout 5e25435, this test will intermittently fail.

// coroutines can produced incorrect output. see example in `base coroutines test 7`
if (standardOutput.startsWith("{")) outputMapper.readValue(standardOutput, ExecutionResult::class.java)
else {
val result = outputMapper.readValue("{" + standardOutput.substringAfter("{"), ExecutionResult::class.java)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe we won't need this workaround, but I wouldn't object to keeping it.

executors/src/main/kotlin/JUnitExecutors.kt Outdated Show resolved Hide resolved
}
catch (e: Exception) {
System.setOut(standardOutput)
print("[\"")
standardOutput.print("[\"")
e.printStackTrace()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that it's writing to PrintStream(errorStream). Then the BAOS is discarded. Should it be sent to stderr?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, hard to say. The code in the catch block seems nonfunctional, regardless of the changes. I tried to reproduce a case when the execution flow reached this part of the code and got a serialization error.

@phiSgr
Copy link
Contributor Author

phiSgr commented Mar 28, 2023

possible to add a test

Is the modified test sufficient?
#618 (comment)

@phiSgr
Copy link
Contributor Author

phiSgr commented Jul 17, 2023

Following up on this.

I wasn't very clear in my previous comment. I have changed the test which now guards against the bug. I don't think a new additional test is needed.

The test that I modified, base coroutines test 7 used to only test for substring match.
Now it is testing for full equality.

@phiSgr
Copy link
Contributor Author

phiSgr commented Dec 10, 2023

This bug has confused another person.

https://stackoverflow.com/questions/77619172/why-does-the-kotlin-coroutine-output-a-b-even-with-a-delay

@phiSgr
Copy link
Contributor Author

phiSgr commented Apr 19, 2024

I found another SO question caused by this bug.
Imagine how many people are confused by this but didn't ask a question.

https://stackoverflow.com/questions/71918773/why-does-coroutine-in-background-execute-before-the-main-thread-in-this-kotlin-c

@nikpachoo
Copy link
Collaborator

@phiSgr, thank you for bringing this up!
We are reviewing the PR, and I hope we can merge it this week.

@nikpachoo
Copy link
Collaborator

No objections from my side.
@phiSgr, thank you for the contribution!

@nikpachoo nikpachoo merged commit 6abd1a1 into JetBrains:master Apr 28, 2024
1 check passed
@phiSgr
Copy link
Contributor Author

phiSgr commented May 1, 2024

@nikpachoo, thanks for merging this.

It seems to me that it's now deployed to https://play.kotlinlang.org only in version 2.0.0-RC2. Will it be back-ported to older versions as well?

@nikpachoo
Copy link
Collaborator

@phiSgr,
1.9.24 version released today contains the fix ✅
There are no plans to back-port it to all the previous versions; their usage is relatively low.
Thank you for your participation ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants