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

Console.Unix: avoid deadlock between LazyInitializer and Console.Out #34297

Merged
merged 32 commits into from
Apr 13, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Mar 30, 2020

Fixes #34197

@stephentoub ptal

@jkotas
Copy link
Member

jkotas commented Mar 30, 2020

Is this complex locking scheme really needed? Would it be better to just create the reader speculatively and let the first one win?

public static TextReader In =>
    s_in ?? Interlocked.CompareExchange(ref s_in, ConsolePal.GetOrCreateReader(), null) ?? s_in

@stephentoub
Copy link
Member

Yeah, I haven't looked at the change yet, but it's what I was thinking here:
#34197 (comment)

@tmds
Copy link
Member Author

tmds commented Mar 30, 2020

Is this complex locking scheme really needed? Would it be better to just create the reader speculatively and let the first one win?

GetOrCreateReader for ConsolePal.Unix looks like this:

internal static TextReader GetOrCreateReader()
{
if (Console.IsInputRedirected)
{
Stream inputStream = OpenStandardInput();
return SyncTextReader.GetSynchronizedTextReader(
inputStream == Stream.Null ?
StreamReader.Null :
new StreamReader(
stream: inputStream,
encoding: Console.InputEncoding,
detectEncodingFromByteOrderMarks: false,
bufferSize: Console.ReadBufferSize,
leaveOpen: true)
);
}
else
{
return StdInReader;
}
}

OpenStandardInput() owns a handle (dup of stdin).
Should we keep the lock?
Or if the lock is removed, should some clean-up be added for the native handle?

@stephentoub
Copy link
Member

Or if the lock is removed, should some clean-up be added for the native handle?

We would want to dispose of any instances that lost the race to be published.

@tmds
Copy link
Member Author

tmds commented Mar 31, 2020

I've changed lazy initialization in Console to use Interlocked.CompareExchange instead of locking.

@tmds
Copy link
Member Author

tmds commented Mar 31, 2020

The StreamReader/Writers leave open the InnerStream that needs to be Disposed in case Interlocked.CompareExchange != null. It results in some more code compared to the LazyInitializer using InternalSyncObject lock.

@tmds
Copy link
Member Author

tmds commented Apr 1, 2020

@stephentoub do you know why underlying StreamReader/StreamWriter used by In, Out, Error get created with leaveOpen: true? It means the user has no way of closing the native handles.

@stephentoub
Copy link
Member

do you know why underlying StreamReader/StreamWriter used by In, Out, Error get created with leaveOpen: true?

I'm not sure what happens on Windows if you close a handle returned by GetStdHandle, and what implications that has on future attempts to use a future result from GetStdHandle; it's possible it stems from that, though I'm just guessing. On Unix we dup the file descriptors, so it should be fine to close them.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks.

@tmds
Copy link
Member Author

tmds commented Apr 7, 2020

@jkotas @danmosemsft @stephentoub thank you for thorough review!

@danmoseley
Copy link
Member

Thank you for your work @tmds.

@VSadov
Copy link
Member

VSadov commented Apr 8, 2020

The change looks good to me.

@VSadov
Copy link
Member

VSadov commented Apr 8, 2020

Re: fences. - There are about 20 uses of Volatile here. It'd be a scary piece of code if they were needed.
I looked at a few of them. In all cases, if I had to add a comment on ordering relationship, it would be something like "we have to ensure that s_in is published after writing to its fields".

That is something you expect every time you write an object that could be seen by other threads.

@tmds
Copy link
Member Author

tmds commented Apr 9, 2020

This should be good to merge.

@stephentoub stephentoub reopened this Apr 12, 2020
@stephentoub
Copy link
Member

Thanks, @tmds.

@stephentoub stephentoub merged commit 59a24e5 into dotnet:master Apr 13, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Console.ForegroundColor deadlocks Console on OSX
7 participants