You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Like Util in #10030, this class is too generic, and used in too many different contexts. Deprecating these methods for removal, and moving the methods to where they are actually used will make it easier to partition the codebase, and ensure reuse actually makes sense.
Additionally, the static close(AutoCloseable) method masks some errors. Guava for example has tools to quietly close Reader and InputStream where it is "safe" to fail to close something that was being read, but when writing this can be different - if the close() failed, this could mean that not all content was written (consider a buffered writer, or where .flush() is called by close() and failed to finish some metadata, etc). There are several places throughout the codebase where we should be allowing a writer or output stream to throw if close fails. A few examples:
com.google.gwt.core.ext.linker.impl.StandardLinkerContext#produceOutput creates a BufferedOutputStream artifactStream, and will log useful errors and throw UnableToCompleteException if there is an IOEx, but if close() fails we see nothing. This means that the last write(s) could fail after the buffer is flushed, but the user would never be notified that they have corrupt content on disk.
SplitPointRecorder.recordSplitPoints and StoryRecorder.recordStoriesImpl write to a GZIPOutputStream, then close and hide exceptions. If the write succeeds, but the close failed, the end of the write could silently fail, with corrupt content on disk.
MinimalRebuildCacheManager.enqueueAsyncWriteDiskCache() has a lambda with a try/catch that closes in the try, and handles IO errors, but still hides write errors that happen on close. This stream is an ObjectOutputStream, and can result in corrupt metadata (bad block header), or fail to write remaining buffered data.
In almost all cases, we can use the try-with-resources pattern to let these failures be thrown as if they were normal write errors.
Like Util in #10030, this class is too generic, and used in too many different contexts. Deprecating these methods for removal, and moving the methods to where they are actually used will make it easier to partition the codebase, and ensure reuse actually makes sense.
Additionally, the static
close(AutoCloseable)
method masks some errors. Guava for example has tools to quietly closeReader
andInputStream
where it is "safe" to fail to close something that was being read, but when writing this can be different - if the close() failed, this could mean that not all content was written (consider a buffered writer, or where .flush() is called by close() and failed to finish some metadata, etc). There are several places throughout the codebase where we should be allowing a writer or output stream to throw if close fails. A few examples:artifactStream
, and will log useful errors and throw UnableToCompleteException if there is an IOEx, but if close() fails we see nothing. This means that the last write(s) could fail after the buffer is flushed, but the user would never be notified that they have corrupt content on disk.In almost all cases, we can use the try-with-resources pattern to let these failures be thrown as if they were normal write errors.
See also #10011.
The text was updated successfully, but these errors were encountered: