Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Commit 84a6820

Browse files
committed
Remove race condition from socket input that could stall reads
- Without the _sync lock, if new data was produced as ConsumingComplete was called, the next "await SocketInput" might never complete despite not all data being examined. - If more data is produced afterward, the stall would be prevented, but this isn't always the case such as during the end of the request.
1 parent 850632a commit 84a6820

File tree

1 file changed

+76
-66
lines changed

1 file changed

+76
-66
lines changed

src/Microsoft.AspNetCore.Server.Kestrel/Http/SocketInput.cs

Lines changed: 76 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ public class SocketInput : ICriticalNotifyCompletion, IDisposable
2828
private MemoryPoolBlock _pinned;
2929

3030
private int _consumingState;
31+
private object _sync = new object();
3132

3233
public SocketInput(MemoryPool memory, IThreadPool threadPool)
3334
{
@@ -58,64 +59,70 @@ public MemoryPoolBlock IncomingStart()
5859

5960
public void IncomingData(byte[] buffer, int offset, int count)
6061
{
61-
if (count > 0)
62+
lock (_sync)
6263
{
63-
if (_tail == null)
64+
if (count > 0)
6465
{
65-
_tail = _memory.Lease();
66-
}
66+
if (_tail == null)
67+
{
68+
_tail = _memory.Lease();
69+
}
70+
71+
var iterator = new MemoryPoolIterator(_tail, _tail.End);
72+
iterator.CopyFrom(buffer, offset, count);
6773

68-
var iterator = new MemoryPoolIterator(_tail, _tail.End);
69-
iterator.CopyFrom(buffer, offset, count);
74+
if (_head == null)
75+
{
76+
_head = _tail;
77+
}
7078

71-
if (_head == null)
79+
_tail = iterator.Block;
80+
}
81+
else
7282
{
73-
_head = _tail;
83+
RemoteIntakeFin = true;
7484
}
7585

76-
_tail = iterator.Block;
86+
Complete();
7787
}
78-
else
79-
{
80-
RemoteIntakeFin = true;
81-
}
82-
83-
Complete();
8488
}
8589

8690
public void IncomingComplete(int count, Exception error)
8791
{
88-
if (_pinned != null)
92+
lock (_sync)
8993
{
90-
_pinned.End += count;
91-
92-
if (_head == null)
94+
if (_pinned != null)
9395
{
94-
_head = _tail = _pinned;
96+
_pinned.End += count;
97+
98+
if (_head == null)
99+
{
100+
_head = _tail = _pinned;
101+
}
102+
else if (_tail == _pinned)
103+
{
104+
// NO-OP: this was a read into unoccupied tail-space
105+
}
106+
else
107+
{
108+
_tail.Next = _pinned;
109+
_tail = _pinned;
110+
}
111+
112+
_pinned = null;
95113
}
96-
else if (_tail == _pinned)
114+
115+
if (count == 0)
97116
{
98-
// NO-OP: this was a read into unoccupied tail-space
117+
RemoteIntakeFin = true;
99118
}
100-
else
119+
if (error != null)
101120
{
102-
_tail.Next = _pinned;
103-
_tail = _pinned;
121+
_awaitableError = error;
104122
}
105123

106-
_pinned = null;
107-
}
108-
109-
if (count == 0)
110-
{
111-
RemoteIntakeFin = true;
124+
Complete();
112125
}
113-
if (error != null)
114-
{
115-
_awaitableError = error;
116-
}
117-
118-
Complete();
119126
}
120127

121128
public void IncomingDeferred()
@@ -162,40 +169,43 @@ public void ConsumingComplete(
162169
MemoryPoolIterator consumed,
163170
MemoryPoolIterator examined)
164171
{
165-
MemoryPoolBlock returnStart = null;
166-
MemoryPoolBlock returnEnd = null;
167-
168-
if (!consumed.IsDefault)
172+
lock (_sync)
169173
{
170-
returnStart = _head;
171-
returnEnd = consumed.Block;
172-
_head = consumed.Block;
173-
_head.Start = consumed.Index;
174-
}
174+
MemoryPoolBlock returnStart = null;
175+
MemoryPoolBlock returnEnd = null;
175176

176-
if (!examined.IsDefault &&
177-
examined.IsEnd &&
178-
RemoteIntakeFin == false &&
179-
_awaitableError == null)
180-
{
181-
_manualResetEvent.Reset();
177+
if (!consumed.IsDefault)
178+
{
179+
returnStart = _head;
180+
returnEnd = consumed.Block;
181+
_head = consumed.Block;
182+
_head.Start = consumed.Index;
183+
}
182184

183-
Interlocked.CompareExchange(
184-
ref _awaitableState,
185-
_awaitableIsNotCompleted,
186-
_awaitableIsCompleted);
187-
}
185+
if (!examined.IsDefault &&
186+
examined.IsEnd &&
187+
RemoteIntakeFin == false &&
188+
_awaitableError == null)
189+
{
190+
_manualResetEvent.Reset();
188191

189-
while (returnStart != returnEnd)
190-
{
191-
var returnBlock = returnStart;
192-
returnStart = returnStart.Next;
193-
returnBlock.Pool.Return(returnBlock);
194-
}
192+
Interlocked.CompareExchange(
193+
ref _awaitableState,
194+
_awaitableIsNotCompleted,
195+
_awaitableIsCompleted);
196+
}
195197

196-
if (Interlocked.CompareExchange(ref _consumingState, 0, 1) != 1)
197-
{
198-
throw new InvalidOperationException("No ongoing consuming operation to complete.");
198+
while (returnStart != returnEnd)
199+
{
200+
var returnBlock = returnStart;
201+
returnStart = returnStart.Next;
202+
returnBlock.Pool.Return(returnBlock);
203+
}
204+
205+
if (Interlocked.CompareExchange(ref _consumingState, 0, 1) != 1)
206+
{
207+
throw new InvalidOperationException("No ongoing consuming operation to complete.");
208+
}
199209
}
200210
}
201211

0 commit comments

Comments
 (0)