"OPC is allready open" Fix

[markjforte]

Hi Lars,

I'm finally migrating my code from your old TXLSRead to TXLSReadWriteII5, and thus I'm using DirectRead=True. If my code throws an exception in OnReadCell, the TXLSReadWriteII5 component is left in a bad state. The next time the user tries to read a spreadsheet, they get an error: "OPC is allready open".

I believe I've implemented a fix, but I'd like you to review and confirm (and perhaps include in a subsequent release). The fix is to add a try finally, as follows:

Code: Select all

procedure TXLSReadXLSX.LoadFromStream(AZIPStream: TStream);
begin
  FTotCells := 0;
  FTotRows := 0;

  // MJF 2014-01-20: Added try-finally to make sure that the OPC is closed
  FManager.FileData.LoadFromStream(AZipStream);
  try
    ReadStyles;
    ReadSST;
    ReadConnections;
    ReadWorkbook;
    ReadSheets;
//  ReadSheetsThread;
    FManager.FileData.ReadUnusedData;
  finally
    FManager.FileData.OPC.Close;
  end;
end;

Does this fix look correct? It seems to work...

Thanks,
Mark

[larsa]

Hello

If there is an exception the component will most certainly be in a very bad state. Trying to catch the exception and just hide it is not a solution. Please provide code sample or a file that reproduces the problem.

[markjforte]

Hi Lars,

I believe you need to look at the code change I made again. It does not catch the exception, nor does it hide it. I added a try finally, to make sure that FManager.FileData.OPC.Close is called. If that call is not made, then the OPC is left open, thereby causing subsequent attempts to read a spreadsheet to immediately fail.

Thanks,
Mark

[larsa]

Hello

If FManager.FileData.OPC.Close not is called because there is an exception, the the solution is to fix the exception and not to hide it in a try ... finally block. Doing so just fixes the symptom, it's not curing the disease.

[markjforte]

Hi Lars,

Again, I'm not hiding any exceptions. If an exception occurs in the OnReadCell event, for whatever reason (typically due to some bad data in the spreadsheet that the user created), that exception should bubble up... For an application with a UI, it should become an exception dialog (we actually catch all exceptions via a MadExcept exception handler, so we can log them, along with the call stack, etc, then we let MadExcept display the exception to the user).

I do not want to try, catch and hide exceptions within our OnReadCell event, as that would make debugging such exceptions extremely difficult.

The way XLSReadWriteII is written now, if an exception occurs in the OnReadCell event (which should be expected), the component is left in an unusable state... If any subsequent read of any spreadsheet is attempted, an "OPS is allready open" exception immediately occurs... Without a fix like the one I proposed, the only solution I see, other than the user closing and reopening the application, is to add code to destroy the XLSReadWriteII component and create a new one, which seems less than ideal.

Hope this helps... Please note that I'm just trying to help make the component more robust.

Thanks!
Mark