Make FileBackedOutputStream use PhantomReference instead of finalize().#8228
Merged
Conversation
502e32d to
c43156e
Compare
…lize()`. This generally makes it more resilient and future-proof, but we still don't recommend relying on GC for object cleanup, in part because any small change (like this one) could upset a delicate balance somewhere. Also, while I'm messing with `reachabilityFence` again: Remove a confused comment from 7eba58f in `GcFinalizationTest`: Once we use `reachabilityFence` directly, we'll eliminate the helper method, and http://errorprone.info/bugpattern/ReachabilityFenceUsage will remain happy because the call to `reachabilityFence` will appear directly inside a `finally` block. (I'm still not sure that `finally` is necessary there, but it's easy, and it's probably reasonable as a default practice.) (I'm also not sure if `finally` is necessary for the new usages of `reachabilityFence` in `FileBackedOutputStream`. I guess the fear would be that we'd allow the `FileOutputStream` to be closed while it's in the process of preparing to throw an exception, and the `close` call could clear something that the exception-construction code wants to read? I do suspect that the new `reachabilityFence` calls are a good idea to have _somewhere_, whether in a `finally` block or not, to ensure that background cleanup doesn't `reset` a `FileBackedOutputStream` during a call to a method like `write` (which might then open a _new_ file) or `close` (which might then not see an exception because the background `reset` sees it instead). Or maybe [`reachabilityFence` isn't necessary because our methods are `synchronized`](https://docs.oracle.com/en/java/javase/25/docs/api/java.base/java/lang/ref/Reference.html#reachabilityFence(java.lang.Object)). (`synchronized` should at least mean that we don't need the fence in a `finally` block, at least not for the reasons that I mentioned above: It won't be possible to `reset` the stream until `write` or `close` exits the critical area.) But presumably it won't hurt anything.) RELNOTES=n/a PiperOrigin-RevId: 875720195
c43156e to
7c6b17c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make
FileBackedOutputStreamusePhantomReferenceinstead offinalize().This generally makes it more resilient and future-proof, but we still don't recommend relying on GC for object cleanup, in part because any small change (like this one) could upset a delicate balance somewhere.
Also, while I'm messing with
reachabilityFenceagain: Remove a confused comment from 7eba58f inGcFinalizationTest: Once we usereachabilityFencedirectly, we'll eliminate the helper method, and http://errorprone.info/bugpattern/ReachabilityFenceUsage will remain happy because the call toreachabilityFencewill appear directly inside afinallyblock. (I'm still not sure thatfinallyis necessary there, but it's easy, and it's probably reasonable as a default practice.)(I'm also not sure if
finallyis necessary for the new usages ofreachabilityFenceinFileBackedOutputStream. I guess the fear would be that we'd allow theFileOutputStreamto be closed while it's in the process of preparing to throw an exception, and theclosecall could clear something that the exception-construction code wants to read? I do suspect that the newreachabilityFencecalls are a good idea to have somewhere, whether in afinallyblock or not, to ensure that background cleanup doesn'tresetaFileBackedOutputStreamduring a call to a method likewrite(which might then open a new file) orclose(which might then not see an exception because the backgroundresetsees it instead). Or maybereachabilityFenceisn't necessary because our methods aresynchronized. (synchronizedshould at least mean that we don't need the fence in afinallyblock, at least not for the reasons that I mentioned above: It won't be possible toresetthe stream untilwriteorcloseexits the critical area.) But presumably it won't hurt anything.)RELNOTES=n/a