Zeoslib Portal

Wild_Pointer wrote:(had to change extension .diff to .txt, diff seems to be considered invalid extension in the forum).

  if PlainDRV.ConsumeInput(Handle)<>1 then
  try
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        isHandled := false;
        if Assigned(FOnError) then FOnError(self, E.Message, isHandled);
        if not isHandled then raise;
      end;
    end;
  finally
    while True do
    begin
      Notify := PlainDRV.Notifies(Handle);
      if Notify = nil then
        Break;
      HandleNotify(Notify);
      PlainDRV.FreeNotify(Notify);
    end;  
  end;

Fr0sT wrote:ZIBEventAlerter.TEventAlert/TErrorEvent should be TZIBEventAlert/TZIBErrorEvent, aren't they? But this would be a breaking change. OTOH, this change is easy to track and correct (build error => examine ZIBEventAlerter unit => find changed identifiers => correct the code). Maybe we could compromise: for 7.2 add TZIB* declarations and mark old ones as deprecated while for 7.3 just change declarations.

marsupilami wrote: if not isHandled then raise;

marsupilami wrote: Also I don't like the way errors are handeled by TZIBEventAlerter. TIBEventThread.SQueEvents uses an exception. But it only queues an error code and not the exception message. I don't see a good reason to not queue the error message. Another thing, we could change in Zeos 7.3. Or we reagrd it as a bug and also do that on Zeos 7.2...

Opinions?

    try
    Status := Parent.PlainDriver.isc_que_events(StatusVector,
      Parent.FNativeHandle, @EventID, EventBufferLen,
      EventBuffer, TISC_CALLBACK(@EventCallback), PVoid(Self));
    except ...
...
procedure EventCallback(P: Pointer; Length: Short; Updated: PAnsiChar); cdecl;
begin
  if (Assigned(P) and Assigned(Updated)) then
  begin
    TIBEventThread(P).UpdateResultBuffer(Length, Updated);
    TIBEventThread(P).SignalEvent;
  end;
end;

...

procedure TIBEventThread.UpdateResultBuffer(Length: UShort; Updated: PAnsiChar);
begin
  Move(Updated[0], ResultBuffer[0], Length);
end;

...

procedure TIBEventThread.SignalEvent;
begin
  EventsReceived := True;
  Signal.SetEvent;
end;
      

Fr0sT wrote: Actually I examined IBX code and it seems Zeos' ZIBEventAlerter.pas code is almost 100% copy of IBEvents.pas... o_O I'm really unsure this is OK without even a note in unit header. Probably both versions are inherited from FIB but I don't know the truth.

This unit has been almost completely re-written as the original code was
not that robust - and I am not even sure if it worked

Fr0sT wrote:There's a;ready ZIBEventAlerter unit with error handling so the change should be done accordingly.
Some code style suggestions:
- const ErrorMessage in TZPgErrorEvent. In 7.3 all ref-counted types in parameters made const or var to avoid excess ref changing.
- FOnError instead of FErrorHandler (why use different name?)
- You do no conversion between PAnsiChar ErrorMsg and String ErrorMessage, with D2009+ there will be warning about implicit conversion; moreover, the DB and native codepages must be considered. Look at ZDbcPostgreSqlUtils.CheckPostgreSQLError function.

marsupilami wrote:

Code: Select all

        if Assigned(FOnError) then FOnError(self, E.Message, isHandled);
        if not isHandled then raise;

    ...
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        if Assigned(FOnError) then FOnError(self, E.Message);
      end;
    end;

Wild_Pointer wrote:I did consider raising exceptions if error is not hadled initially, but then realized that this way I would change current behavior of the component and decided playing safe. The other reason - I've found no way (strangely) to handle exceptions from timer events on Lazarus... TApplicationProperties did not catch the exception...

Fr0sT wrote:Actually I examined IBX code and it seems Zeos' ZIBEventAlerter.pas code is almost 100% copy of IBEvents.pas... o_O I'm really unsure this is OK without even a note in unit header. Probably both versions are inherited from FIB but I don't know the truth.

Wild_Pointer wrote:I did consider raising exceptions if error is not hadled initially, but then realized that this way I would change current behavior of the component and decided playing safe. The other reason - I've found no way (strangely) to handle exceptions from timer events on Lazarus... TApplicationProperties did not catch the exception...
That said maybe isHandled is not needed? Lets do:

Code: Select all

    ...
    try
      CheckPostgreSQLError(...);
    except
      on E: EZSQLException do begin
        if Assigned(FOnError) then FOnError(self, E.Message);
      end;
    end;

• Disable the timer
• Do the work
• Check for errors, throw an exception if there is an error and no error handler
• reenable the timer if there is no error -> For me there is no reason to check for new events if we already had an error.

Fr0sT wrote:I doubt that throwing an exception is necessary here.

Fr0sT wrote:Moreover, I personally dislike the very idea of doing potentially long blocking operations in OnTimer as it will freeze the main thread until the polling is completed. IBX way with background thread seems smarter.

Fr0sT wrote:But here an architectural decision is required: what level of changes is enough. Simple patch is easy but some issues remain. The deepest change would be a common interface for background thread and event component, from which any DBC-specific implementation would inherit.

marsupilami wrote: Hmmm honestly I don't care if components change behaviour for Zeos 7.3. For Zeos 7.2 it is a different story. Your proposed code just swallows all exceptions that happen - at least all EZSQLExceptions. So if I don't install an error handler everything will look as if it works although it doesn't.

marsupilami wrote: I like programs and components that hit me in the face when something is wrong. This way I at least know that something is wrong. Keeping that in mind my sugestion for this is like this:

• Disable the timer
• Do the work
• Check for errors, throw an exception if there is an error and no error handler
• reenable the timer if there is no error -> For me there is no reason to check for new events if we already had an error.

• Alerter stays Active when connection is closed (causes AV)
• Does not word in 64bit programs. TZPostgreSQLNotify record should not be packed. Current implementation raises AV in 64bit programs because of allignment problems. (Other structure to record mappings should be revised too.)
• The same component should not be used to manage notifications and to fire events. It causes too much confusion on how to use it. For example the secondary TPgEventAlerter object connected to the main through Processor property doesn't use Interval property. The user must know there should not be used several independent TPgEventAlerter objects connected to single TZConnection... The user of the component must know it from the inside to use it correctly.
• I'm not sure if there should be components in ZeosLib that are related to single DB management system. IMHO there should be one TEventAlerter than has the same properties for all the DBMS. If extra properties are needed they can be provided the same way it is in TZConnection - "Properties".

marsupilami wrote: How did you check the thing about Exceptions with Lazarus? Today I did a very simplistic test with a TTimer that raised an exception. For me it behaved like it should - I got the exception error message. Did I misunderstand you there?

marsupilami wrote: For PostgreSQL I am not sure if older versions of libpq are thread safe. Honestly I don't even know that for the current version. We should check that before we try to implem,ent threading there.

One thread restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same connection object. (If you need to run concurrent commands, use multiple connections.)

marsupilami wrote: we might be in trouble because these files were distributed under the InterBase Public License Version 1.0. That't what the original IBEvents.pas says. I doubt that the InterBase Public Licens allows to reuse their code with any other license we wish.

How should we handle errors that are not taken care of? How should the developer know that something went wrong?

Honestly I think that if a check for errors shows some errors a potentially freezing main thread will be your smallest problem. Also raising errors can only be done in the main thread because our components live there. For PostgreSQL I am not sure if older versions of libpq are thread safe. Honestly I don't even know that for the current version. We should check that before we try to implem,ent threading there.

Hmm. I doubt that all libraries we use are thread safe. Even if they are, libraries for future drivers might not be. So if we do something architectural about events I suggest we keep a structure where the driver gets to decide if it wants to use a separate thread for checking for events.