[JENKINS-60916] Revert try-with-resources for S3GzipCallable #127
Conversation
This would close the InputStream prematurely, failing on the scheduled uploads who expected an open stream. Note that as the upload continues after the return, therefore the zip-stream must not (yet) be closed in this context. Fixes JENKINS-60916
semicolons
MMeent
changed the title
| // so we cannot use its AutoCloseable behaviour with a | ||
| // try-with-resources statement, as that would likely | ||
| // close the stream before the upload has succeeded. | ||
| final InputStream gzippedStream = new FileInputStream(localFile); |
There was a problem hiding this comment.
I do not know this with certainty, but as it is an async stream acceptor, I do believe that it at least should accept ownership of the stream, and thus close it when appropriate, and thus not be incorrect usage here. The current usage of the API is (AFAICS) no different from how it is used in the non-gzipped variant in https://github.com/jenkinsci/s3-plugin/blob/master/src/main/java/hudson/plugins/s3/callable/S3UploadCallable.java#L25
There was a problem hiding this comment.
I'm fine with reverting if the current behavior is not working, but I think it should be investigated whether a leak is happening and fix it.
There was a problem hiding this comment.
Seems like the stream is being closed after upload:
Uploads.getInstance() returns an Uploads, which closes the passed inputstream after completion here
Unless my static analisys is incorrect, this stream will be closed once passed to Uploads#startUploading
There was a problem hiding this comment.
Great, thanks for taking the time to validate it!
|
I'm not a maintainer here |
|
I pinged you as you are one of the 2 ping-able committers of the last release; so my excuses for this incorrect ping. |
|
@alecharp any luck to get this merged? |
rsandell
changed the title
This would close the InputStream prematurely, failing on the scheduled uploads who expected an open stream.
Note that as the upload continues after the return, therefore the zip-stream must not (yet) be closed in this context.
Fixes JENKINS-60916