TestServer returns error with response when server ends with pending read

[JamesNK]

Fix flaky test - #19242

[jkotalik]

nit: why change this?

[JamesNK]

I put a for loop of 1,000,000 around the test logic to see if the error went away. There was a race condition about the order the properties are set that comes up here:

private void CheckAborted()
{
    if (_aborted)
    {
        throw new IOException(string.Empty, _abortException);
    }
}

You could get an IOException with a null inner exception.

[jkotalik]

This kind of pattern scares me. Most people expect to set a bool like _aborted at the at the top of a method called Aborted. Would it be better just to lock when checking aborted and setting the aborted Exception?

[JamesNK]

Is locking necessary here? We just need to put the class in the right state before setting _aborted to true.

I could add a comment to Aborted if you'd like.

[jkotalik]

eh, I think it's fine. Not worth rerunning CI for.

[Tratcher]

If something is concurrently accessing this state then it should be locked.

[JamesNK]

This?

        private readonly object _abortLock = new object();

        internal void Abort(Exception innerException)
        {
            Contract.Requires(innerException != null);

            lock (_abortLock)
            {
                _abortException = innerException;
                _aborted = true;
                _pipe.Reader.CancelPendingRead();
            }
        }

        private void CheckAborted()
        {
            lock (_abortLock)
            {
                if (_aborted)
                {
                    throw new IOException(string.Empty, _abortException);
                }
            }
        }