TRODiscoveryException should provide the server IP


(obones) #1

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.


(EvgenyK) #2

it won’t help you because lServer isn’t assigned:

  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

type
  TMyCustomDiscoveryClient = class(TROComponent, IROBroadcastNotification)
  private
    fChannel: TROBroadcastChannel;
    fMessage: TROMessage;
    fServerList: TStrings;
    fServiceName: string;
    fOnNewServersFound: TNotifyEvent;
    fDiscoveryService: IRODiscoveryService_Async;
  end;

TMyCustomDiscoveryClient(RODiscoveryClient1).fDiscoveryService := TIRODiscoveryService_AsyncProxy.Create('IRODiscoveryService', RODiscoveryClient1.Message, RODiscoveryClient1.Channel);

above code will allow to ask for IRODiscoveryService instead of ROServiceDiscovery


(obones) #3

Yes, I know, but I’m talking about the ServerIP parameter that would already give enough information.

Thanks for the workaround, I’ll see if it’s applicable in the long term.


(EvgenyK) #4

when you need this parameter? when only exception is raised or when any response is received?


(obones) #5

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.


(EvgenyK) #6

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.

What is more suitable for you?


(obones) #7

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.


(EvgenyK) #8

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;

(obones) #9

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.


(EvgenyK) #10

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


(RemObjects) #11

Thanks, logged as bugs://80351


(RemObjects) #12

bugs://80351 got closed with status fixed.