Jump to content
Angus Robertson

Error handling

Recommended Posts

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

 

Share this post


Link to post

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

 

Share this post


Link to post

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.

Share this post


Link to post

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

 

Share this post


Link to post
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.

 

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
×