[GitHub] [lucenenet] NightOwl888 commented on issue #478: Disposing IndexWriter doesn't check if it's already disposed/closed

[GitBox <gi...@apache.org>]

NightOwl888 commented on issue #478:
URL: https://github.com/apache/lucenenet/issues/478#issuecomment-830537225


   Okay, what alternative would you expect? To be able to reopen? To throw an exception?
   
   Per the [IDisposable documentation](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?redirectedfrom=MSDN&view=net-5.0#remarks):
   
   > If an object's [Dispose](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose) method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its [Dispose](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose) method is called multiple times. Instance methods other than [Dispose](https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose) can throw an [ObjectDisposedException](https://docs.microsoft.com/en-us/dotnet/api/system.objectdisposedexception) when resources are already disposed.
   
   Note it says "must", not "should". Also, since an `IndexWriter` cannot be reopened after it is disposed, it seems like this is the right approach.
   
   Do keep in mind that using the new `using` statement from C# 8.0, `Dispose` may be called automatically by the compiler even after it had already been called explicitly.
   
   ```c#
   { // Block for scope
       using var x = new MyDisposable();
       // Do some work (there may be an exception)
       x.Dispose();
       // Do some more work
   } // Dispose is called here whether the above Dispose() gets called or not
   ```
   
   This is a common pattern when using `IndexWriter.GetReader()`, which must be called before disposing the `IndexWriter`.
   
   ```c#
   using IndexWriter writer = new IndexWriter(directory, config);
   // Add some docs
   using IndexReader reader = writer.GetReader();
   writer.Dispose();
   // Use the reader...
   ```
   
   It would be awkward to work with if there were an exception thrown just because we want to use a `using` statement, since the purpose is to ensure `Dispose` is called *at least* once regardless of whether an exception occurs or the operation is successful.
   
   As for the overload with `waitForMerges`, it conflicts with the protected `Dispose(bool)` overload that should exist in .NET for subclasses to implement. I am considering to rename it `DisposeNoWait()` and add an extension method to patch the current calls to the public `Dispose(bool)` to call either `Dispose()` or `DisposeNoWait()` depending on the `bool` value.
   
   What use case are you trying to solve for that you expect this to work differently?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org