Following the cleanup of discovery files, preexisting servers do not support the ROServiceDiscovery interface as it was renamed from IROServiceDiscovery and this leads to exceptions being raised when doing discovery with up to date clients.
If the code is left as is, the first exception raised prevents further responses to be processed by the broadcast client, thus giving the impression that no server is available on the network.
We can avoid this by giving a handler for TRODiscoveryClient.OnDiscoveryException which prevents the exception from escaping out of ResponseReceived, thus allowing any further response to be processed.
While this is fine, the handler for OnDiscoveryException does not receive any information about which server triggered the exception, which is kinda sad because the ServerIP parameter could be passed along. Would it be possible to include it in a further release? Maybe in a second event to avoid crashes when the handler is assigned via a DFM.
As a side note, I would like to know how to gracefully handle the fact that the interface name can either be IROServiceDiscovery (legacy) or ROServiceDiscovery (current) because it’s near impossible to update all clients at once.
try
lServer := fDiscoveryService.Retrieve_FindService(lOptions);
except
on E: Exception do begin
if Assigned(FOnDiscoveryException) then begin
FOnDiscoveryException(Self, E);
...
I can suggest to you another temporary workaround: manually assign fDiscoveryService that uses legacy interface name, like
Only when exception is raised. This way I can log which IP sends me bogus replies, while also allowing further responses to be processed.
If I don’t give any exception handler, the first response that triggers an exception prevents any other one from arriving, and if I give an exception handler, I have no details about the machine that created the bogus reply.
I think, we can modify exception message and provide server IP in it, by other hand, we could create a new exception like ERODiscoveryException and provide ServerIP in it.
Changing the event signature is risky because someone may have assigned a handler via the DFM which will crash at runtime because of the missing new parameter.
Adding a new exception class would be nice, but I don’t see how it would be given to the existing OnDiscoveryException handler.
To me, there should be a DoDiscoveryException function that returns True if a handler was given and add a second type of handler (like TRODiscoveryExceptionEx) that would have the new parameter. Maybe something like that
function DoDiscoveryException(const ServerIP: string; E: Exception): Boolean;
begin
Result := False;
if Assigned(FOnDiscoveryException) then begin
FOnDiscoveryException(Self, E);
Exit(True);
end;
if Assigned(FOnDiscoveryExceptionEx) then begin
FOnDiscoveryExceptionEx(Self, ServerIP, E);
Exit(True);
end
end;
and then the code inside TROCustomDiscoveryClient.ResponseReceived could be written like this:
try
lServer := fDiscoveryService.Retrieve_FindService(lOptions);
except
on E: Exception do begin
if DoDiscoveryException(ServerIP, E) then begin
exit;
end
else begin
raise;
end;
end;
end;
That’s just a suggestion, but the source issue is that any exception raised by fDiscoveryService.Retrieve_FindService prevents any further response to be processed.
I mean something else, I can add ServerIP in to E.Message like
on E: Exception do begin
if Assigned(FOnDiscoveryException) then begin
E.Message := E.Message + ', serverip ='+serverip; // ofc, we can't change r/o property, but it shows my idea
FOnDiscoveryException(Self, E);
or into a new exception (ERODiscoveryException), like
on E: Exception do begin
newexception := ERODiscoveryException.Create(E.Message, ServerIP);
if Assigned(FOnDiscoveryException) then begin
FOnDiscoveryException(Self, newexception);
later, it can be used like
if aException is ERODiscoveryException then
lserverIP := ERODiscoveryException(aException).ServerIP;
Well, yes, but if I want to do some special processing on serverip itself, I can’t get it directly. And parsing the exception message is something that I avoid like the plague because it is not time proof.
Well, to avoid a memory leak, you would have to raise that new exception in the except part of the try construct. And from experience, I know it does not work properly with ancient Delphi versions where inner exceptions simply crash the runtime.
If you were to simply create a temporary exception object, then I would not be able to use a tool like EurekaLog or madExcept to write down details of where and how the exception occurred.
But in the end, what really matters is that any exception occurring while discovering a server should not prevent further answers from being processed.
in madExcept, you can catch original exception w/o ServerIP part. I think, it will be enough for you.
if new exception wasn’t raised in OnDiscoveryException event, it will be destroyed via newexception.free so no memory leaks will be here