"close all lob streams before closing the resultset"

[aehimself]

I encountered a massive memory leak when I terminated a worker thread which was in the process of downloading LOBs. Terminating calls ZConnection.AbortOperation (which properly ended the download) but when I attempted to close the dataset before freeing it up I got the exception mentioned in the subject.

Therefore, all lob streams, the query, the connection and the worker thread failed to release - resulting the memory leak.
I am wondering, if we know that there are stuff to be closed, why aren't we closing it? I created a patch and it seems to do the job just fine.

As usual, the patch is sent via a GitHub pull request.

[aehimself]

Jan,

Does your comment mean that the patch is not going to be applied? Should I start looking for an other place to close the lobs?

[aehimself]

If we are not fine manually closing the lobs instead of throwing an exception, I don't know if I'll find where the missing closing statement should be. I removed the exception so I have a nice stack of the leak:

System.TObject.NewInstance Line 17836 000000000040d5d0 System.pas, line 17836
System.TInterfacedObject.NewInstance Line 39917 0000000000419e54 System.pas, line 39917
System._ClassCreate Line 19214 000000000040e21e System.pas, line 19214
Zdbcoracleresultset.TZOracleClob.TZOracleClob Line 3424 0000000000c65fec ZDbcOracleResultSet.pas, line 3424
Zdbcoracleresultset.TZOracleAbstractResultSet_A.GetBlob Line 2284 0000000000c60c96 ZDbcOracleResultSet.pas, line 2284
Zdbccache.TZRowAccessor.FillFromFromResultSet Line 1347 00000000008bcc25 ZDbcCache.pas, line 1347
Zdbcoracleresultset.TZOracleRowAccessor.FillFromFromResultSet Line 3866 0000000000c67e51 ZDbcOracleResultSet.pas, line 3866
Zdbccachedresultset.TZCachedResultSet.Fetch Line 2418 00000000008d70c6 ZDbcCachedResultSet.pas, line 2418
Zdbccachedresultset.TZCachedResultSet.MoveAbsolute Line 2648 00000000008d7a57 ZDbcCachedResultSet.pas, line 2648
Zdbcresultset.TZAbstractResultSet.Next Line 2567 00000000008ad351 ZDbcResultSet.pas, line 2567
Zabstractrodataset.TZAbstractRODataset.FetchOneRow Line 1997 0000000000d58078 ZAbstractRODataset.pas, line 1997
Zabstractrodataset.TZAbstractRODataset.FetchRows Line 1970 0000000000d57f61 ZAbstractRODataset.pas, line 1970
Zabstractrodataset.TZAbstractRODataset.GetRecordCount Line 3790 0000000000d5ebe6 ZAbstractRODataset.pas, line 3790
Usearchinfirxmlsform.TSearcherThread.Execute Line 169 00000000010da6e2 uSearchInFirXMLsForm.pas, line 169
System.Classes.ThreadProc Line 15509 00000000004c69d0 System.Classes.pas, line 15509
System.ThreadWrapper Line 25370 00000000004108ba System.pas, line 25370

The problematic (I think) will be procedure TZRowAccessor.FillFromFromResultSet(const ResultSet: IZResultSet;{$IFDEF AUTOREFCOUNT}const {$ENDIF}IndexPairList: TZIndexPairList);

Code: Select all

        stAsciiStream, stUnicodeStream, stBinaryStream: begin
            PIZLob(Data)^ := ResultSet.GetBlob(ResultSetIndex);
            if FCachedLobs then
              PIZLob(Data)^ := GetAsCachedLob(PIZLob(Data)^);
          end;

Data is a PPointer, which is initialized like this:

Code: Select all

    P := @FBuffer.Columns[FColumnOffsets[ColumnIndex{$IFNDEF GENERIC_INDEX} - 1{$ENDIF}]];
    {$IFDEF RangeCheckEnabled}{$R+}{$ENDIF}
    Data := Pointer(PAnsiChar(P)+1);

So I suspect FBuffer.Columns does not get freed up somewhere? I also made the observation that the exception is raised when I close my TZQuery. In the AFTERCLOSE event it looks like this:

Code: Select all

procedure TZCachedResultSet.AfterClose;
begin
  inherited AfterClose;
  If Assigned(FResultset) then begin
    FResultset.Close;
    FResultSet := nil;
  end;
end;

And the exception is raised when FResultSet is being closed, not the main ZQuery object.

I will attempt to debug it further but I do believe that if we know that something needs to be freed, we should free it instead of throwing an exception.

[marsupilami]

Hello aehimself,

I am sorry - maybe I have been too short in my explanation. What I really intended to say is that this exception is expected behavior. At least I think that it is. I seem to remember that we had users, who expected a BLOB-Stream to work even after closing the dataset. This is why the exception about closing the BLOB-Streams was introduced in the first place. Zeos expects users to manage the BLOB-Streams correctly because they directly link to data in the database. At least for some drivers.
So - in my opinion - raising the exception is the right thing to do. It tells the user that something is wrong in the program logic. Creating situations where a BLOB-Stream object becomes invalid because we allow closing the dataset seems to be wrong to me. But I might be overruled by Egonhugeist.

At the minimum I think that this change needs some discussion.

Best regards,

Jan

[marsupilami]

Hello aehimself,

I am sorry - maybe I have been too short in my explanation. What I really intended to say is that this exception is expected behavior. At least I think that it is. I seem to remember that we had users, who expected a BLOB-Stream to work even after closing the dataset. Zeos expects users to manage the BLOB-Streams correctly because they directly link to data in the database. At least for some drivers (Firebird?).
So - in my opinion - raising the exception is the right thing to do. It tells the user that something is wrong in the program logic. Creating situations where a BLOB-Stream object becomes invalid because we allow closing the dataset seems to be wrong to me. But I might be overruled by Egonhugeist.

At the minimum I think that this change needs some discussion.

Best regards,

Jan

[aehimself]

Jan,

I am open to discussion but I think you misunderstand the issue. I'm not doing anything special in my worker thread, except:
- Open a large dataset with CLOB fields
- Get the String the CLOB contains, and do some work with it (dataset.FieldByName.AsString)
- Once all is done, close the dataset and destroy everything

I am using uncached lobs (data is being downloaded when accessed, not when opening the dataset as far as I can tell the difference) and I am not accessing LOB streams directly.
My problem is that .AsString seems to be correctly surrounded by Try..Finally..stream.Free blocks and the issue still arises if Connection.AbortOperation is called.
And again - the lob stream is not open in the main ZQuery object. They are kept open in FDataSet used for caching (?).

[marsupilami]

In that case, it sounds like a bug in Zeos to me. Zeos should free all BLOB-Streams internally as soon as possible. The blob stream interface shouldn't be cached. We only should cache BLOB data.

Using your Stack I would guess that one of the red lines is a good point for starting a search:

System.TObject.NewInstance Line 17836 000000000040d5d0 System.pas, line 17836
System.TInterfacedObject.NewInstance Line 39917 0000000000419e54 System.pas, line 39917
System._ClassCreate Line 19214 000000000040e21e System.pas, line 19214
Zdbcoracleresultset.TZOracleClob.TZOracleClob Line 3424 0000000000c65fec ZDbcOracleResultSet.pas, line 3424
Zdbcoracleresultset.TZOracleAbstractResultSet_A.GetBlob Line 2284 0000000000c60c96 ZDbcOracleResultSet.pas, line 2284
Zdbccache.TZRowAccessor.FillFromFromResultSet Line 1347 00000000008bcc25 ZDbcCache.pas, line 1347
Zdbcoracleresultset.TZOracleRowAccessor.FillFromFromResultSet Line 3866 0000000000c67e51 ZDbcOracleResultSet.pas, line 3866
Zdbccachedresultset.TZCachedResultSet.Fetch Line 2418 00000000008d70c6 ZDbcCachedResultSet.pas, line 2418
Zdbccachedresultset.TZCachedResultSet.MoveAbsolute Line 2648 00000000008d7a57 ZDbcCachedResultSet.pas, line 2648
Zdbcresultset.TZAbstractResultSet.Next Line 2567 00000000008ad351 ZDbcResultSet.pas, line 2567
Zabstractrodataset.TZAbstractRODataset.FetchOneRow Line 1997 0000000000d58078 ZAbstractRODataset.pas, line 1997
Zabstractrodataset.TZAbstractRODataset.FetchRows Line 1970 0000000000d57f61 ZAbstractRODataset.pas, line 1970
Zabstractrodataset.TZAbstractRODataset.GetRecordCount Line 3790 0000000000d5ebe6 ZAbstractRODataset.pas, line 3790
Usearchinfirxmlsform.TSearcherThread.Execute Line 169 00000000010da6e2 uSearchInFirXMLsForm.pas, line 169
System.Classes.ThreadProc Line 15509 00000000004c69d0 System.Classes.pas, line 15509
System.ThreadWrapper Line 25370 00000000004108ba System.pas, line 25370

I seem to rememeber that doCachedLobs doesn't have an effect on Oracle - we always cache blob data there.

Does that only happen when you use AbortOperation?

[aehimself]

We only should cache BLOB data.

This is something I do not necessary agree with. I'm mainly working with CLOB fields and accessing the information there indeed takes a tremendous amount of time. Caching CLOBs is not a bad idea, if you have to re-process a dataset over and over again.

I seem to rememeber that doCachedLobs doesn't have an effect on Oracle - we always cache blob data there.

That is more than correct. See this forum post for confirmation.

Does that only happen when you use AbortOperation?

Yes; and only if everything is in a background thread. If connection is in the VCL thread, only .Open is called from a background (like what ZMethodInThread does) the leak did not occur until now. I might be wrong about this, maybe I just never hit the Abort button in the right time until now.

Using your Stack I would guess that one of the red lines is a good point for starting a search:

System.TObject.NewInstance Line 17836 000000000040d5d0 System.pas, line 17836
System.TInterfacedObject.NewInstance Line 39917 0000000000419e54 System.pas, line 39917
System._ClassCreate Line 19214 000000000040e21e System.pas, line 19214
Zdbcoracleresultset.TZOracleClob.TZOracleClob Line 3424 0000000000c65fec ZDbcOracleResultSet.pas, line 3424
Zdbcoracleresultset.TZOracleAbstractResultSet_A.GetBlob Line 2284 0000000000c60c96 ZDbcOracleResultSet.pas, line 2284
Zdbccache.TZRowAccessor.FillFromFromResultSet Line 1347 00000000008bcc25 ZDbcCache.pas, line 1347
Zdbcoracleresultset.TZOracleRowAccessor.FillFromFromResultSet Line 3866 0000000000c67e51 ZDbcOracleResultSet.pas, line 3866
Zdbccachedresultset.TZCachedResultSet.Fetch Line 2418 00000000008d70c6 ZDbcCachedResultSet.pas, line 2418
Zdbccachedresultset.TZCachedResultSet.MoveAbsolute Line 2648 00000000008d7a57 ZDbcCachedResultSet.pas, line 2648
Zdbcresultset.TZAbstractResultSet.Next Line 2567 00000000008ad351 ZDbcResultSet.pas, line 2567
Zabstractrodataset.TZAbstractRODataset.FetchOneRow Line 1997 0000000000d58078 ZAbstractRODataset.pas, line 1997
Zabstractrodataset.TZAbstractRODataset.FetchRows Line 1970 0000000000d57f61 ZAbstractRODataset.pas, line 1970
Zabstractrodataset.TZAbstractRODataset.GetRecordCount Line 3790 0000000000d5ebe6 ZAbstractRODataset.pas, line 3790
Usearchinfirxmlsform.TSearcherThread.Execute Line 169 00000000010da6e2 uSearchInFirXMLsForm.pas, line 169
System.Classes.ThreadProc Line 15509 00000000004c69d0 System.Classes.pas, line 15509
System.ThreadWrapper Line 25370 00000000004108ba System.pas, line 25370

Yes, TZRowAccessor.FillFromFromResultSet is suspicious to me as well:

The problematic (I think) will be procedure TZRowAccessor.FillFromFromResultSet(const ResultSet: IZResultSet;{$IFDEF AUTOREFCOUNT}const {$ENDIF}IndexPairList: TZIndexPairList);

Code: Select all

        stAsciiStream, stUnicodeStream, stBinaryStream: begin
            PIZLob(Data)^ := ResultSet.GetBlob(ResultSetIndex);
            if FCachedLobs then
              PIZLob(Data)^ := GetAsCachedLob(PIZLob(Data)^);
          end;

Data is a PPointer, which is initialized like this:

Code: Select all

    P := @FBuffer.Columns[FColumnOffsets[ColumnIndex{$IFNDEF GENERIC_INDEX} - 1{$ENDIF}]];
    {$IFDEF RangeCheckEnabled}{$R+}{$ENDIF}
    Data := Pointer(PAnsiChar(P)+1);

So I suspect FBuffer.Columns does not get freed up somewhere?

My problem with this is I have no idea where / when this should be freed up. I'll do some tests, maybe I'll find something.

[aehimself]

I think I found the issue; the LOB was indeed not closed. I just have no idea why - maybe Michael can shine some light on it?
I pushed two more commits to the pull request, restoring the exception, fixing a slight Try...Finally issue and fixing the main leak problem.

Originally, the method looked like this:

Code: Select all

destructor TZAbstracOracleLobStream.Destroy;
begin
  FOwnerLob.FLobStream := nil;
  if (FOwnerLob.FLobLocator <> nil) then
    Close;
  inherited;
end;

Since the LOB closing penalty fix Michael introduced LobIsOpen and started to check that when closing... so I changed FOwnerLob.FLobLocator <> nil to FOwnerLob.LobIsOpen and everything started to work fine. Although since this check is in .Close, I simply removed the condition.

So the real question. What is a LobLocator and how it can be nil while the lobstream is still open?