Skip to content

TextRunner accidentally disposes System.Out #4319

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

Closed
jeroenvervaeke opened this issue Mar 24, 2023 · 8 comments · Fixed by #4317
Closed

TextRunner accidentally disposes System.Out #4319

jeroenvervaeke opened this issue Mar 24, 2023 · 8 comments · Fixed by #4317
Labels
is:bug requires:backport Requires backportint to the v3 branch
Milestone

Comments

@jeroenvervaeke
Copy link

It is possible to replace System.Out with your own implementation of TextWriter.
One reason why you would want to do that is if you want to both capture all output to a stream and write it to console.

I was using this approach and I stumbled upon an issue when using TextRunner.
When using TextRunner, and not providing a file, TextRunner defaults to System.Out.
When TextRunner is done running it disposes the underlying stream that it was writing to. This is correct behaviour when using files, but not desired behaviour when using System.Out.
See: source code

I modified ConsoleColorWriter so Dispose is a NOP. (The base class disposes the writer provided by ConsoleColorWriter = System.Out).

I've also added a unit test to verify that the fix works. If you want a code example of what crashes, check the unit test.

@jeroenvervaeke
Copy link
Author

Created a PR for this issue: #4318

@mr-russ
Copy link
Contributor

mr-russ commented Mar 25, 2023

How does PR #4317 and your pull request relate? I've not looked in detail as I don't understand the implications of each. They appear on the surface to do similar things. Can you provide some comments about how they work together, if they are the same or completely different?

@jeroenvervaeke
Copy link
Author

Apparently my colleague and I both created a PR in our spare time for the same issue without telling each other about it. We can close my PR in favor of the PR that @normj created

@normj
Copy link
Contributor

normj commented Mar 25, 2023

Whoops sorry about the miscommunication @jeroenvervaeke and @mr-russ and duplicate PRs

@stevenaw
Copy link
Member

Not a problem! It's always welcome to have bugs filed, and having two PRs submitted to fix it is never a bad thing either.

PS: I enjoyed your recent Lambda GC article @normj , great read.

@stevenaw
Copy link
Member

Oops, I just noticed that this was the only open issue for this. I'm going to reopen it to track the PR fix

@OsirisTerje
Copy link
Member

The current PR #4317 to main should go there, and it would be nice if @normj would add another PR to merge to the 3.13 branch.

@normj
Copy link
Contributor

normj commented Mar 26, 2023

I update the PR with a test and I can create a PR for 3.13 once I know I did this PR correctly.

@stevenaw Great to hear you like the blog post!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:bug requires:backport Requires backportint to the v3 branch
Projects
None yet
5 participants