paulirwin commented on issue #271: URL: https://github.com/apache/lucenenet/issues/271#issuecomment-2510521423
> So, what I gather, you are saying a disposable class would be injected like this? That's the idea, yeah. Although your example of the icu-dotnet case is a good thought experiment. After some thinking, I think we can made TokenStream IDisposable as well as ICloseable, although the virtual `Dispose(boolean)` method would _only_ be an extension point for anyone that needs an example like that. In almost all other cases, Close should be used instead. And as I noted above, we would be able to get away with not having any of our TokenStream subclasses override Dispose. To do this, I think we would need to make TokenStreamComponents implement IDisposable and dispose of its two fields, although we would have to note that because there is no guarantee that the sink ends up disposing of its source, that we would have to allow for Dispose to be idempotent and able to be called multiple times, because if we just i.e. not dispose of the TokenStreamComponents source field (assuming the sink would dispose it), then there's a chance that it might not, and then something could leak. We could add a trivial check to not redundantly call Dispose on the sink if it ReferenceEquals the source, but other than that I think we have to allow for Dispose to be called more than once. We then need to change DisposableThreadLocal to dispose of all of its Values that are IDisposable. I'll prototype this and see how it goes. Note that I do not think we should do the suggestion earlier in this thread of throwing if any of the other TokenStream methods are called on it after it has been disposed. The Closeable implementation does not do this in the upstream code, and we'd have to override many methods from AttributeSource, and make IncrementToken virtual instead of abstract, which would significantly deviate from upstream, and cause a performance penalty, just to prevent someone from doing this in their custom types (as again, we would not be overriding Dispose in our code). I think we should make that the responsibility of the implementer if they care about that, and just let it be. > Can we make those [TestRandomChains] tests pass without having disposable analysis components? They do pass, even with high iteration counts. I've removed the AwaitsFix attribute from them. > Can we make the tests that use a single analyzer instance for a whole test fixture class fail independently without getting cascade failures? This also seems to work fine. I think it was due to wrapping the IDisposables in using statements previously like you said, and now I've replaced those with `try`/`finally` and calling `.Close()` in the finally block. This is effectively the same as a using block, which the compiler lowers to `try`/`finally` and calling `.Dispose()` in the finally block. So the behavior is basically the same, just the semantics have changed: Close means reusable, Dispose means you're done with the object. We already effectively were treating disposed objects as reusable, this just creates the distinction in meaning. > Even on TextReader implementations that own file handles? Do we have any tests that you know of that do this that I can confirm? But again, if they were passing previously, they're still passing now, as there's basically little difference in runtime behavior than before. -- 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. To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
