Using WaitForPipeDrain is not cross-platform way

[rstm-sf]

Hello!

Using WaitForPipeDrain is not cross-platform way. It's only windows

MsBuildPipeLogger/src/MsBuildPipeLogger.Logger/PipeWriter.cs

Line 75 in 39819e5

_pipeStream.WaitForPipeDrain();

[daveaglick]

So I've apparently forgotten about this for [checks watch] almost a year. It would be easy enough to surround the call with an OS check, but I'm a little confused why this hasn't blown up in practice since it's used in Buildalyzer and that gets tested cross-platform. Is it possible this is a no-op on non-Windows platforms? And any idea what the non-Windows wait for drain equivalent should be?

[xoofx]

Is it possible this is a no-op on non-Windows platforms?

Nope, it's a throw.

https://github.com/dotnet/runtime/blob/ba4eae0b96f2a569050bd315b0b51609993e237c/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs#L354

I'm a little confused why this hasn't blown up in practice

Because it is in a try/catch, so likely, the _pipeStream.Dispose(); is never called on non-Windows platforms.

MsBuildPipeLogger/src/MsBuildPipeLogger.Logger/PipeWriter.cs

Lines 71 to 80 in 39819e5

	try
	{
	_queue.CompleteAdding();
	_doneProcessing.WaitOne();
	_pipeStream.WaitForPipeDrain();
	_pipeStream.Dispose();
	}
	catch
	{
	}

[xoofx]

Fixed by PR #7

[daveaglick]

Because it is in a try/catch, so likely, the _pipeStream.Dispose(); is never called on non-Windows platforms.

Oh, duh 🤦‍♂️. And not getting to the dispose could have unpleasant side-effects, so glad this got reported and fixed. Thanks both!