Error handling

[Angus Robertson 520]

ICS has a core function, probably unchanged for 20 years, that starts:

 

{ Return -1 if error, else return number of byte written                    }
function TCustomWSocket.Send(Data : TWSocketData; Len : Integer) : Integer;
begin
    if (FState <> wsConnected) and (FState <> wsSocksConnected) then begin
        WSocket_Synchronized_WSASetLastError(WSAENOTCONN);
        SocketError('Send');
        Result := -1;
        Exit;
    end;

....

 

My problem is that the SocketError function is quite complex, and has options to trigger event handlers with exception information.  In a server, those error handlers are often considered fatal and may raise an exception or terminate the application, despite the function being documented as returning -1 for an error. 

 

For servers in particular, it is common for a connection to be lost unexpectedly, and this should not really trigger major exceptions. 

 

So I'm proposing to remove SocketError, so that error never returns an exception but returns as documented. 

 

But will this break existing applications that may rely on an exception?

 

Angus

 

[FPiette 363]

This would probably break a lot of code!

[Angus Robertson 520]

SocketError( } is important for raising terminal errors through the onBgExecption event, errors that can leave a server unresponsive, which is why I restart it. 

 

But SocketError( } treats all errors the same, fatal and non-fatal.  In my case, a web hacker closed an HTTP connection before the server could answer, thus no connection while trying to send a response, and a server restart.   Not common, once or twice a year from millions of connections, but still a bug. 

 

On reflection, I think we should raise a local exception in Send(), which the same behaviour as SocketError( } if an error handler event is not specified, which is less likely to cause backward compatibility issues.

 

Angus

 

[Remy Lebeau 1257]

A client disconnect should never be treated as a fatal error that kills the server.  Just close the client socket and move on.  Let the server continue serving other clients normally.

[Angus Robertson 520]

To be fair, it's my implementation of the ICS web server component that deliberately kills the server on an unexpected fatal error so that Service Manager Controller restarts it, because I know from 15 years experience of such servers that very rarely the server stops responding after a fatal error. 

 

ICS normally treats disconnection correctly, many other places don't cause the fatal error handler to be called, but just very rarely does this specific instance occur. 

 

For the moment, I'm now ignoring this particular error in the fatal error handler, but it's better never to happen. 

 

Angus

 

[Remy Lebeau 1257]

2 hours ago, Angus Robertson said:

To be fair, it's my implementation of the ICS web server component that deliberately kills the server on an unexpected fatal error so that Service Manager Controller restarts it

That makes sense for true unexpected/fatal errors, however...

2 hours ago, Angus Robertson said:

ICS normally treats disconnection correctly, many other places don't cause the fatal error handler to be called, but just very rarely does this specific instance occur. 

If a simple disconnect is being treated as a fatal error, that sounds like a bug that needs fixing.