mORMot Open Source

franfj

Member

Registered: 2018-03-28

Posts: 12

TAsynchConnections.ConnectionRemove from outside of OnRead causes AV

I'm developing a server which has the singularity that must close all the connections (the clients stay connected until the server closes the connection, it's an Asterisk FastAGI server).

If I close a connection using "ConnectionRemove" I get random errors (AV, memory corruption, etc.) and analyzing the code, it seems that the "lock" performed on the connection only locks "connections" array but not the internal connection that is going to be removed:

function TAsynchConnections.ConnectionRemove(aHandle: TAsynchConnectionHandle): boolean;
var i: integer;
    conn: TAsynchConnection;
begin
  result := false;
  if (self=nil) or Terminated or (aHandle<=0) then
    exit;
  conn := ConnectionFindLocked(aHandle,@i);
  if conn<>nil then
    try
      if not fClients.Stop(conn) then
        fLog.Add.Log(sllDebug,'ConnectionRemove: Stop=false for %',[conn],self);
      result := ConnectionDelete(conn,i);
    finally
      fConnectionLock.UnLock;
    end;
  if not result then
    fLog.Add.Log(sllTrace,'ConnectionRemove(%)=false',[aHandle],self);
end;

Since the connection may be processing by the "pseRead" thread, the ConnectionDelete call will destroy it, and then the "Read" thread may cause AccessViolations.

Having that in mind, I tried adding a "slot.Lock" to avoid destroying the connection while is being read:

function TAsynchConnections.ConnectionRemove(aHandle: TAsynchConnectionHandle): boolean;
var i: integer;
    conn: TAsynchConnection;
    locked: boolean;
begin
  result := false;
  if (self=nil) or Terminated or (aHandle<=0) then
    exit;
  conn := ConnectionFindLocked(aHandle,@i);
  if conn<>nil then
    try
      locked := conn.fSlot.TryLock(100);
      if not locked then begin
        fLog.Add.Log(sllDebug,'ConnectionRemove: fSlot.TryLock=false for %',[conn],self);
        result := false;
        Exit;
      end;
      if not fClients.Stop(conn) then
        fLog.Add.Log(sllDebug,'ConnectionRemove: Stop=false for %',[conn],self);
      result := ConnectionDelete(conn,i);
    finally
      fConnectionLock.UnLock;
      if locked then
        conn.fSlot.UnLock;
    end;
  if not result then
    fLog.Add.Log(sllTrace,'ConnectionRemove(%)=false',[aHandle],self);
end;

But this has two disadvantages:
* It breaks the ability to perform "ConnectionRemove" inside "OnRead" thread, because connection "lock" is not a mutex/criticalsection, just an Integer which increments on each Lock.
* It still gives random Access Violations and Memory corruption errors (which I haven't found why).

The final solution I've applied is simple, just use a Boolean value inside connection to indicate that I want to close that connection, and let "OnRead" close the connection returning "sorClose".

  TMyConnection=class(TAsynchConnection)
  private
    FDisconnectOnNextRead: boolean;
  protected
    function OnRead(Sender: TAsynchConnections): TPollAsynchSocketOnRead; override;
  ...
  end;
---------
procedure TMyConnection.Disconnect;
begin
   FDisconnectOnNextRead := True; 
   // I usually send some message to the client here, that causes a OnRead later, then it gets disconnected
end;

function TMyConnection.OnRead(Sender: TAsynchConnections):TPollAsynchSocketOnRead;
begin
  if FDisconnectOnNextRead then
  begin
    result := sorClose; // could be ConnectionRemove, but it's easier to do this way
    Exit;
  end;
  // do the Read then return sorContinue
end;

It's not a "nice" way to do, but it's simple, and works.

Having that in mind, I think that the documentation of the function must say that only MUST be executed from a method inside the "read" thread (like TAsynchConnection.OnRead )

Remove an handle from the internal list, and close its connection
- could be executed e.g. from a TAsynchConnection.OnRead method
https://synopse.info/files/html/api-1.1 … TIONREMOVE

Anyone has any suggestion on how to improve this?

Thanks!

Last edited by franfj (2018-04-26 10:35:59)