Skip to content

Commit 3392893

Browse files
authored
Avoid UnobservedTaskExceptions from HttpConnection.PrepareForReuse (#104335)
* Add regression test * Fix the bug
1 parent b652b39 commit 3392893

File tree

3 files changed

+78
-19
lines changed

3 files changed

+78
-19
lines changed

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,15 @@ public bool PrepareForReuse(bool async)
169169
_readAheadTask = _stream.ReadAsync(_readBuffer.AvailableMemory);
170170
#pragma warning restore CA2012
171171

172-
return !_readAheadTask.IsCompleted;
172+
// If the read-ahead task already completed, we can't reuse the connection.
173+
// We're still responsible for observing potential exceptions thrown by the read-ahead task to avoid leaking unobserved exceptions.
174+
if (_readAheadTask.IsCompleted)
175+
{
176+
LogExceptions(_readAheadTask.AsTask());
177+
return false;
178+
}
179+
180+
return true;
173181
}
174182
catch (Exception error)
175183
{

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionBase.cs

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -181,24 +181,6 @@ internal static int ParseStatusCode(ReadOnlySpan<byte> value)
181181
return 100 * (status1 - '0') + 10 * (status2 - '0') + (status3 - '0');
182182
}
183183

184-
/// <summary>Awaits a task, ignoring any resulting exceptions.</summary>
185-
internal static void IgnoreExceptions(ValueTask<int> task)
186-
{
187-
// Avoid TaskScheduler.UnobservedTaskException firing for any exceptions.
188-
if (task.IsCompleted)
189-
{
190-
if (task.IsFaulted)
191-
{
192-
_ = task.AsTask().Exception;
193-
}
194-
}
195-
else
196-
{
197-
task.AsTask().ContinueWith(static t => _ = t.Exception,
198-
CancellationToken.None, TaskContinuationOptions.ExecuteSynchronously | TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default);
199-
}
200-
}
201-
202184
/// <summary>Awaits a task, logging any resulting exceptions (which are otherwise ignored).</summary>
203185
internal void LogExceptions(Task task)
204186
{

src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,75 @@ static async Task MakeARequestWithoutDisposingTheHandlerAsync()
9999
}
100100
}
101101

102+
[Fact]
103+
public async Task ReadAheadTaskOnConnectionReuse_ExceptionsAreObserved()
104+
{
105+
bool seenUnobservedExceptions = false;
106+
107+
EventHandler<UnobservedTaskExceptionEventArgs> eventHandler = (_, e) =>
108+
{
109+
if (e.Exception.InnerException?.Message == nameof(ReadAheadTaskOnConnectionReuse_ExceptionsAreObserved))
110+
{
111+
_output.WriteLine(e.Exception.ToString());
112+
seenUnobservedExceptions = true;
113+
}
114+
};
115+
116+
TaskScheduler.UnobservedTaskException += eventHandler;
117+
try
118+
{
119+
const string FirstResponse = "HTTP/1.1 200 OK\r\nContent-Length: 3\r\n\r\nfoo";
120+
int firstResponseOffset = 0;
121+
122+
TaskCompletionSource firstWriteCompleted = new(TaskCreationOptions.RunContinuationsAsynchronously);
123+
124+
using var client = new HttpClient(new SocketsHttpHandler
125+
{
126+
PooledConnectionIdleTimeout = Timeout.InfiniteTimeSpan,
127+
ConnectCallback = (_, _) =>
128+
{
129+
return ValueTask.FromResult<Stream>(new DelegateDelegatingStream(Stream.Null)
130+
{
131+
ReadAsyncMemoryFunc = async (buffer, ct) =>
132+
{
133+
if (firstResponseOffset < FirstResponse.Length)
134+
{
135+
await firstWriteCompleted.Task.WaitAsync(ct);
136+
int toWrite = Math.Min(buffer.Length, FirstResponse.Length - firstResponseOffset);
137+
Assert.Equal(toWrite, Encoding.ASCII.GetBytes(FirstResponse.AsSpan(firstResponseOffset, toWrite), buffer.Span));
138+
firstResponseOffset += toWrite;
139+
return toWrite;
140+
}
141+
142+
throw new Exception(nameof(ReadAheadTaskOnConnectionReuse_ExceptionsAreObserved));
143+
},
144+
WriteAsyncMemoryFunc = (_, _) =>
145+
{
146+
firstWriteCompleted.TrySetResult();
147+
return default;
148+
}
149+
});
150+
}
151+
});
152+
153+
// The first request succeeds.
154+
Assert.Equal("foo", await client.GetStringAsync("http://foo"));
155+
156+
// The connection throws an exception after the first request.
157+
// No matter what happens with the second request, we should always be observing the connection exception.
158+
await Assert.ThrowsAsync<Exception>(() => client.GetAsync("http://foo"));
159+
160+
GC.Collect();
161+
GC.WaitForPendingFinalizers();
162+
}
163+
finally
164+
{
165+
TaskScheduler.UnobservedTaskException -= eventHandler;
166+
}
167+
168+
Assert.False(seenUnobservedExceptions);
169+
}
170+
102171
[Fact]
103172
public async Task ExecutionContext_Suppressed_Success()
104173
{

0 commit comments

Comments
 (0)