Problem with TAsyncClient OnRead ( Never Called When Receive Data From Server)

[Coldzer0]

Hello @synopse

This is related to https://synopse.info/forum/viewtopic.php?id=6038

I Created a Client Example that uses the TAsyncClient Class but didn't work as expected.
https://gist.github.com/Coldzer0/8c98a02721979abe116c9089791f55e4

The problem is because the TAsyncClient.Execute Never release the readpoll lock for the polling threads.

So I Created this sample
https://gist.github.com/Coldzer0/226e0d4ac724e0c511895b4485594e28

It uses TAsyncConnection and I uses this code to add the connection to the internal reading poll.

    // assign the new connection to the internal reading poll.
    If Clients.Start(aconnection) Then
    Begin
      // release atpReadPoll lock to handle new subscription ASAP
      ThreadPollingWakeup(fClients.PollRead.PollForPendingEvents(0));

      // Send 2MB As Test - we should receive it
      WriteString(aConnection, DupeString('A', 1024*1024*2));
      Result := True;
    End

So I think the TAsyncClient.Execute needs to be updated to handle such case.

Regards.

[Coldzer0]

Hello @synopse

Is there any new update related to this issue ?

Thanks.

[Coldzer0]

@synopse

So After sometime reading the code I've some concern regards the TAsyncClient.

I think this class should serve the Connection as only a Async ThreadPoll for Read & Write.

So the Idea behind the the aClientsCount should change.
My idea is you should be able to add your own Socket Connection to the TPollAsyncSockets and still works fine.

Take my new code here as example
https://gist.github.com/Coldzer0/c91b6e144ac0c3227f884c3b355326ac
This should work fine But the​Fix only works if I add aClientsCount bigger than 0 And It should be available .

So What I propose is the next change and everything should work as expected.

diff --git a/src/net/mormot.net.async.pas b/src/net/mormot.net.async.pas
index d9a500b7..589196ee 100644
--- a/src/net/mormot.net.async.pas
+++ b/src/net/mormot.net.async.pas
@@ -587,7 +587,8 @@ type
   // - will use a TAsyncConnection inherited class to maintain connection state
   // of each connected client
   TAsyncClient = class(TAsyncConnections)
-  protected
+  Protected
+    Function ConnectionCreate(aSocket: TNetSocket; Const aRemoteIp: RawUtf8; out aConnection: TAsyncConnection): Boolean; Override;
     procedure Execute; override;
   public
     /// start the TCP client connections, connecting to the supplied IP server
@@ -1714,12 +1715,7 @@ begin
     raise EAsyncConnections.CreateUtf8('%: %:% connection failure (%)',
       [self, fThreadClients.Address, fThreadClients.Port, ToText(res)^]);
   connection := nil;
-  if ConnectionCreate(client, {ip=}'', connection) then
-  begin
-    if fThreadReadPoll <> nil then
-      fThreadReadPoll.fEvent.SetEvent // awake reading thread(s)
-  end
-  else
+  if not ConnectionCreate(client, {ip=}'', connection) then
     client.ShutdownAndClose({rdwr=}false);
 end;
 
@@ -2282,6 +2278,23 @@ begin
     aLog, aOptions, aThreadPoolCount);
 end;
 
+function TAsyncClient.ConnectionCreate(aSocket: TNetSocket;
+  const aRemoteIp: RawUtf8; out aConnection: TAsyncConnection): Boolean;
+begin
+  result := false;
+  if not Terminated then
+  begin
+    aConnection := fConnectionClass.Create(self, aRemoteIp);
+    if inherited ConnectionAdd(aSocket, aConnection) then
+    begin
+      result := Clients.Start(aconnection); // assign a new connection to the internal reading poll
+      if Result then
+        if fThreadReadPoll <> nil then
+          fThreadReadPoll.fEvent.SetEvent; // awake reading thread(s)
+    end;
+  end;
+end;
+
 procedure TAsyncClient.Execute;
 var
   notif: TPollSocketResult;

Regards.

[Coldzer0]

Hello @synopse

Did you check my comment above ?

[synopse]

This changes the default behavior of ConnectionCreate, which creates the connection, but don't add it, nor start it. As its name states.
So your patch breaks the Liskov substitution principle of OOP.

For a new client connection, ConnectionCreate + Clients.Start() should be called explicitly.
I have modified the source so that

• TAsyncConnections.ThreadClientsConnect can be called to create a new client on need;
• fClients.Start will awake reading thread(s) using the new fClients.OnStart event

[Coldzer0]

Thanks @synopse for the update.

I'm not professional when it's about OOP so excuse my lake of knowledge in this area. (Thanks for the explanation)
The last commit fix the problem
By using Self.Clients.Start(connection); After connecting.

Or just set the clients count to what we want.

Thanks.