#1 2016-07-13 16:52:07

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

[Discussion] Close the TCrtSocket in case recv() return 0

While implementing a firefox remote debug protocol we discover an issue in TCrtSocket.TrySockRecv.

If connection is closed by peer recv(..) return a 0 as described here

This is normal situation, and my opinion - we should close a socket.

So, in TCrtSocket.TrySockRecv:

//instead of

if Size<=0 then
  exit

//should be

if Size<=0 then begin
  if Size=0 then
    Close;
  exit;
end; 

In current realization I can't determinate - is socket closed or some errors exists.

AB's answer:

AB wrote:

I'm not sure it is correct patch.

If there is a timeout (no data received in the expected period), then Size=0 also, and we definitively should not close the socket.

Please create a forum thread for further discussion.
It is better not to use the tickets for discussion!

Does anyone have any ideas about the right way?

Offline

#2 2016-07-13 18:12:07

danielkuettner
Member
From: Germany
Registered: 2014-08-06
Posts: 330

Re: [Discussion] Close the TCrtSocket in case recv() return 0

After reading this:

https://msdn.microsoft.com/de-de/librar … s.85).aspx

I guess, you could/should close the socket.

Offline

#3 2016-07-14 08:21:08

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Offline

#4 2016-07-14 08:32:18

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Thanks a lot!

Offline

#5 2016-07-19 11:40:35

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Last 3 days I fight with a socket timeout problem described [ff91d71434].
But I discover root of my problem not a timeout. On the some Windows network configurations (looks like when VPN adapters is added)  calls to HttpGet randomly (1 time work, 3 times not) fail with a connect timeout

initResult := HttpGet(CONTROLLER.serverConfig.HTTPServer.getFullURL + 'init', nil);

After rewriting to TWinHTTP

initResult := TWinHTTP.Get(CONTROLLER.serverConfig.HTTPServer.getFullURL + 'init', '', true, nil);

everything is OK.

@AB - My suggestion is to modify SynCrtSock.HttpGet to always use a TWinHTTP on Windows (just move a one ifdef up a little). It can save a lot of debugging time for others smile

Offline

#6 2016-07-19 12:16:53

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

... but this doesn't fix anything in the TCrtSocket class, anyway...

Offline

#7 2016-07-19 12:26:16

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Yes, but I can't provide a detail description of a TCrtSocket  problem, so no idea how to fix it. On the servers where problem exists I can't debug, on my developer environments I can't reproduce a problem - I try on 3 different computers w/o results

Last edited by mpv (2016-07-19 12:28:36)

Offline

#8 2016-07-19 12:45:04

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Perhaps some issue at IP/socket level, depending on the TCP/IP stack parameters (and firewall)?
IP exhausting ports?
IP not reusing existing socket?

Did you try keeping the socket alive between requests?

Offline

#9 2016-07-19 13:33:08

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

I will try to install Delphi on the problem machine and debug. On weekend. The problem randomly occurrence on the first request, so this is not a keep-alive

Offline

#10 2020-08-20 13:44:49

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

@ab - the issue with socket close detection is again in trunk. I think that for a long time - after AsynchRecv is added , but noticed just now.
In current implementation of TCrtSocket.TrySockRecv - see this link 

   read := AsynchRecv(fSock,Buffer,read);
      if read<=0 then begin // no more to read, or socket issue?
        {$ifdef SYNCRTDEBUGLOW}
        TSynLog.Add.Log(sllCustom2, 'TrySockRecv: sock=% AsynchRecv=% %',
          [Sock,read,SocketErrorMessage],self);
        {$endif}
        if WSAIsFatalError then begin
          Close; // connection broken or socket closed gracefully
          exit;
        end;
        if StopBeforeLength then
          break;
      end 

in case connection is closed by counterpart for blocked sockets read result is 0 and  WSAIsFatalError is false (WSAGetLastError returns 0), so socket not closing and server continue to wait for data.
The correct fix here is to close a socket in case we receive 0 bytes - as a side effect this remove unnecessary call to WSAGetLastError

if (socketIsBlocking and (read=0) {connection closed}) or WSAIsFatalError then begin
          Close; // connection broken or socket closed gracefully
          exit;
        end;                                       

But we need to know socket Is Blocking or not. The question - how can we ensure socket is blocking (without syscall)? May be add a property to TCrtSocket? Or we can expect socket in TCrtSocket.TrySockRecv is always blocking?

Last edited by mpv (2020-08-20 13:47:31)

Offline

#11 2020-08-20 14:15:56

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

I guess it is safe to write

if (read=0) or WSAIsFatalError then begin

Even of Windows, IIRC, it means end of file, i.e. "if the connection has been gracefully closed, the return value is zero"
- see https://docs.microsoft.com/en-us/window … nsock-recv and https://man7.org/linux/man-pages/man2/read.2.html

On non-blocking sockets, it returns -1 and an error code as EAGAIN or EWOULDBLOCK.

Offline

#12 2020-08-20 14:26:47

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Offline

#13 2020-08-20 14:32:42

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Please, merge my PR #334 also. I need to override a socket timeout on bind operation (set it to 0 in my case)

Offline

#14 2020-08-20 14:47:40

mpv
Member
From: Ukraine
Registered: 2012-03-24
Posts: 1,564
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

BTW the same issue is with TrySndLow.
It's not critical BUT - sometimes client (browser in my case) closes connection without waiting for response (I think it's happens than user reload page and some request is in pending state).
In this case I got exceptions in log (raised inside TCrtSocket.SndLow).

But I do not see a how to gracefully sovle it (without braking changes).
The only possible improvement is to exit ASAP if sent=0 (but exception remains)

Offline

#15 2020-08-21 06:39:22

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,375
Website

Re: [Discussion] Close the TCrtSocket in case recv() return 0

Offline

Board footer

Powered by FluxBB