Skip to content

Commit

Permalink
fixed TAsyncConnections.IdleEverySecond potential deadlock
Browse files Browse the repository at this point in the history
- now notify TWebSocketAsyncConnection.OnLastOperationIdle outside of fConnectionLock
- if a WS which fails at socket level, it deletes the connection so tries to acquire fConnectionLock which is already in ReadOnlyLock state
  • Loading branch information
Arnaud Bouchez committed Sep 13, 2023
1 parent e62c8e8 commit 78ca8ca
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/mormot.commit.inc
@@ -1 +1 @@
'2.1.5868'
'2.1.5869'
32 changes: 20 additions & 12 deletions src/net/mormot.net.async.pas
Expand Up @@ -2666,8 +2666,9 @@ procedure TAsyncConnections.LogVerbose(connection: TPollAsyncConnection;

procedure TAsyncConnections.IdleEverySecond;
var
i: PtrInt;
notified, gced: PtrInt;
i, notified, gced: PtrInt;
idle: array of TAsyncConnection;
idles: integer;
sec, allowed, gc: cardinal;
c: TAsyncConnection;
begin
Expand All @@ -2677,6 +2678,7 @@ procedure TAsyncConnections.IdleEverySecond;
exit;
// call TAsyncConnection.ReleaseMemoryOnIdle and OnLastOperationIdle events
// and update TAsyncConnection.fLastOperation when needed
idles := 0;
notified := 0;
gced := 0;
sec := fLastOperationSec; // 32-bit second resolution is fine
Expand All @@ -2699,30 +2701,36 @@ procedure TAsyncConnections.IdleEverySecond;
end
else
begin
// check if some events should be triggerred on this inactive connection
// inactive connection: check if some events should be triggerred
// e.g. TWebSocketAsyncConnection would send ping/pong heartbeats
if (gc <> 0) and
(c.fLastOperation < gc) then
inc(gced, c.ReleaseMemoryOnIdle);
if (allowed <> 0) and
(c.fLastOperation < allowed) then
try
if c.OnLastOperationIdle(sec) then
inc(notified);
if Terminated then
break;
except
end;
ObjArrayAddCount(idle, c, idles); // calls below, outside the lock
end;
end;
finally
fConnectionLock.ReadOnlyUnLock;
end;
// OnLastOperationIdle should be called outside of fConnectionLock because if
// Write() fails, it calls ConnectionDelete() and its WriteLock (e.g. WS ping)
for i := 0 to idles - 1 do
try
c := idle[i];
if c.OnLastOperationIdle(sec) then
inc(notified);
if Terminated then
break;
except
// this overriden method should fail silently
end;
if (acoVerboseLog in fOptions) and
((notified <> 0) or
(gced <> 0)) then
DoLog(sllTrace, 'IdleEverySecond % notif=% GC=%',
[fConnectionClass, notified, KBNoSpace(gced)], self);
DoLog(sllTrace, 'IdleEverySecond % idle=% notif=% GC=%',
[fConnectionClass, idles, notified, KBNoSpace(gced)], self);
end;

procedure TAsyncConnections.ProcessIdleTix(Sender: TObject; NowTix: Int64);
Expand Down
4 changes: 3 additions & 1 deletion src/net/mormot.net.ws.async.pas
Expand Up @@ -278,8 +278,10 @@ function TWebSocketAsyncConnection.OnLastOperationIdle(
delaysec := TWebSocketAsyncServer(fServer).fSettings.HeartbeatDelay shr 10;
if nowsec < delaysec + fLastOperation then
exit; // nothing to send (most common case)
// it is time to notify the other end that we are still alive
fProcess.SendPing; // Write will change fWasActive, then fLastOperation
result := true;
// warning: Write calls ConnectionDelete() so fConnectionLock on socket error
result := true; // notify TAsyncConnections.IdleEverySecond
end;

function TWebSocketAsyncConnection.DecodeHeaders: integer;
Expand Down

0 comments on commit 78ca8ca

Please sign in to comment.