[JENKINS-20187] - Handle growing files when creating a tar file. #2419

alvarolobato · 2016-06-22T14:35:23Z

JENKINS-20187

If the file grows while being included on the tar file (typical case of a log file), it will only be included the original size when the operation was started and included on the header of the tar entry.

Removed dependency from org.apache.tools.tar.TarConstants in favor of org.apache.commons.compress.archivers.tar.TarConstants

@reviewbybees esp @jglick @jtnord @amuniz

ghost · 2016-06-22T14:35:41Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2016-06-22T14:45:19Z

core/src/main/java/hudson/util/io/TarArchiver.java

+                        }
+                    }
+                } catch (IOException e) {// log the exception in any case
+                    LOGGER.log(Level.SEVERE, "Error writing to tar file from: " + file, e);


Why log it here? IMO it is the responsibility of the caller to log errors, or not.

If you merely wish to record the filename causing the error (in case it is not otherwise obvious), you could

throw new IOException("Error writing to tar file from " + file + ": " + e, e);

The reason to log is the same as the comment below, there are some cases were you get another exception an loose the actual exception.

It would be better to use Throwable.addSuppressed then.

I believe the case here was from the fact that if this errors, then the close that happens later on (in the caller or somewhere I don;t recall off the top of my head will almost certainly throw an exception due to an un-closed entry which will then just walk all over this exception

Changing that to a try-with-resources should fix that issue, as well as making the code cleaner, then just go with Jesse's suggestion of catch, wrap and re-throw.

Sure, if the code idiom can be converted to try-with-resources, then by all means do it.

BTW some Java IDEs offer this conversion as a hint.

Actually the exception is thrown on the final of the caller not on this one
(the reference I put before) and only when we don't have the final to close
the entry. I'll convert it to try with resources, and see if we can use
Jesse's suggestion.
El 22/6/2016 20:13, "Jesse Glick" notifications@github.com escribió:

In core/src/main/java/hudson/util/io/TarArchiver.java
#2419 (comment):

FileInputStream in = new FileInputStream(file);

try {

int len;

int read = 0;

while ((len = in.read(buf)) >= 0) {

if (len + read <= size) { // ensure we don't write more bytes than the declared when we created

// the entry

tar.write(buf, 0, len);

read += len;

} else {

tar.write(buf, 0, (int) size - read);

break;

}

}

} catch (IOException e) {// log the exception in any case

LOGGER.log(Level.SEVERE, "Error writing to tar file from: " + file, e);

Sure, if the code idiom can be converted to try-with-resources, then by
all means do it.

BTW some Java IDEs offer this conversion as a hint.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/jenkinsci/jenkins/pull/2419/files/64b039bc7a79590877c1800a9aa1bb23549a214f#r68105507,
or mute the thread
https://github.com/notifications/unsubscribe/AQXNBPHtA-FuIO2fKadsRtDHxnCRfZzIks5qOXtAgaJpZM4I72ij
.

jglick · 2016-06-22T14:46:57Z

🐝

oleg-nenashev · 2016-06-22T14:48:50Z

Honestly I don't see a reason to perform such graceful handling. In many cases it will also lead to the truncated & invalid files.

I'd vote for just failing file archiving if it changes during the operation.

alvarolobato · 2016-06-22T14:58:10Z

@oleg-nenashev the reason is not graceful handling, is getting the actual exception to the caller.

jglick · 2016-06-22T16:14:44Z

core/src/main/java/hudson/util/io/TarArchiver.java

+                            read += len;
+                        } else {
+                            tar.write(buf, 0, (int) size - read);
+                            break;


Easier to use BoundedInputStream.

- Remove exception login - Throw new exception with filename and addSuppresed

alvarolobato · 2016-06-23T07:59:06Z

I've changed the try for try-with resources and also added the AddSuppresed. As I suspected if I don't close the entry, even if we add the suppressed, the exception will be masked by the tar.close on the caller, we will get this without closing the entry:
java.io.IOException: This archives contains unclosed entries. at hudson.util.io.TarArchiverTest.growingFileTar(TarArchiverTest.java:132)

instead of this (if closing the entry):
hudson.os.PosixException: native error calling stat: No such file or directory non_existent_file at hudson.util.io.TarArchiverTest.growingFileTar(TarArchiverTest.java:132)

oleg-nenashev · 2016-06-26T11:03:09Z

Anyway it's better than the original behavior. Hence 🐝

alvarolobato · 2016-06-27T08:54:08Z

@reviewbybees done

oleg-nenashev · 2016-06-29T10:24:04Z

I'm going to merge it today if there is no -1s

jglick · 2016-06-29T18:01:34Z

core/src/main/java/hudson/util/io/TarArchiver.java

+        try {
+            if (!file.isDirectory()) {
+                // ensure we don't write more bytes than the declared when we created the entry
+                try (BoundedInputStream in = new BoundedInputStream(new FileInputStream(file), size)) {


🐜 safer is to use

try (InputStream is = new FileInputStream(file); BoundedInputStream in = new BoundedInputStream(is, size)) {

so that you can be guaranteed the FileInputStream is closed.

jglick · 2016-06-29T18:03:02Z

🐝

ghost · 2016-06-29T18:06:15Z

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

oleg-nenashev · 2016-06-30T10:21:49Z

@reviewbybees done
@alvarolobato Do you want to address @jglick's comments?

alvarolobato · 2016-06-30T10:45:21Z

Yes, let me a few hours.

alvarolobato · 2016-06-30T10:57:27Z

@oleg-nenashev, I've addressed @jglick comments.

oleg-nenashev · 2016-06-30T19:47:54Z

re-:bee: ready to merge
cc @jenkinsci/code-reviewers

…aven-builder * 'master' of github.com:jenkinsci/jenkins: (161 commits) updated changelog for release [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release jenkins-2.12 [JENKINS-25416][JENKINS-28790] - Fix the parametersReferencedFromPropertiesShouldRetainBackslashes() test, which relies on the legacy behavior (jenkinsci#2435) Noting jenkinsci#2417, jenkinsci#2428, jenkinsci#2424, jenkinsci#2390, jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976 [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally (jenkinsci#1976) [JENKINS-20187] - Handle growing files when creating a tar file. (jenkinsci#2419) Noting merge of JENKINS-36280 [JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored (jenkinsci#2426) [FIXED JENKINS-36232] NPE during SCM polling (jenkinsci#2425) [JENKINS-21486] Fixing optional dependency version resolution. (jenkinsci#2421) Stray import. [JENKINS-36280] Add some tests Update BUILD_TAG description to mention the replacement of slashes with dashes (jenkinsci#2417) Tests did not match naming pattern so were never being executed [JENKINS-32027] Avoiding to refresh codemirror through the layoutUpdateCallback (jenkinsci#2390) [FIXED JENKINS-36277] Check that process working dir exists (jenkinsci#2424) [JENKINS-36280] Address review comments Make BulkChange auto-closeable [FIXED JENKINS-36280] Enable DescriptorVisibilityFilter for Slave's ComputerLauncher, RetentionStrategy and NodeProperty ...

…kinsci#2419) * [JENKINS-20187] - Handle growing files when creating a tar file. * - Introduce try-with-resources - Remove exception login - Throw new exception with filename and addSuppresed * Address jglick comments

…jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976

@issue

…i#2424) [JENKINS-32027] Avoiding to refresh codemirror through the layoutUpdateCallback (jenkinsci#2390) Update BUILD_TAG description to mention the replacement of slashes with dashes (jenkinsci#2417) Divergence introduced with 117d69b. Tests did not match naming pattern so were never being executed [JENKINS-21486] Fixing optional dependency version resolution. (jenkinsci#2421) Without it version of optional dependencies is set to "X.Y;resolution:=optional" which causes problems when using hudson.util.VersionNumber comparison methods. Make BulkChange auto-closeable so that it can be used as below: try (BulkChange bc = new BulkChange(o)) { ... bc.commit(); } Stray import. [FIXED JENKINS-36232] NPE during SCM polling (jenkinsci#2425) * [FIXED JENKINS-36232] NPE during SCM polling * add test to trigger the "no veto" code path that has the NPE * incorporate remark on using @issue reference [JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored (jenkinsci#2426) * [FIXED JENKINS-35906] Ensure that SCMDescriptor.newInstance overrides are honored. * [FIXED JENKINS-36043] Work around fragile form submission design in multi-branch-project-plugin. [FIXED JENKINS-36280] Enable DescriptorVisibilityFilter for Slave's ComputerLauncher, RetentionStrategy and NodeProperty [JENKINS-36280] Address review comments [JENKINS-36280] Add some tests The tests pass locally, but they will not run until jenkinsci#2429 is merged Noting merge of JENKINS-36280 [JENKINS-20187] - Handle growing files when creating a tar file. (jenkinsci#2419) * [JENKINS-20187] - Handle growing files when creating a tar file. * - Introduce try-with-resources - Remove exception login - Throw new exception with filename and addSuppresed * Address jglick comments [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally (jenkinsci#1976) * [FIXED JENKINS-25416][JENKINS-28790] Do not inject build variables into maven process optionally * [JENKINS-25416][JENKINS-28790] Fixed typo in test method name * [JENKINS-25416][JENKINS-28790] Suggest alternatives in help * [JENKINS-25416][JENKINS-28790] Use less hackish approach to preserving legacy behaviour Noting jenkinsci#2417, jenkinsci#2428, jenkinsci#2424, jenkinsci#2390, jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976 [JENKINS-25416][JENKINS-28790] - Fix the parametersReferencedFromPropertiesShouldRetainBackslashes() test, which relies on the legacy behavior (jenkinsci#2435) The test fails after jenkinsci@84ee34f [maven-release-plugin] prepare release jenkins-2.12 [maven-release-plugin] prepare for next development iteration updated changelog for release merge upstream merge upstream removed variable fixed form neabling issue merge upstream

[JENKINS-20187] - Handle growing files when creating a tar file.

64b039b

jglick reviewed Jun 22, 2016
View reviewed changes

- Introduce try-with-resources

be056fe

- Remove exception login - Throw new exception with filename and addSuppresed

oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label Jun 26, 2016

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Jun 29, 2016

jglick reviewed Jun 29, 2016
View reviewed changes

Address jglick comments

8562f96

oleg-nenashev merged commit 5fdf33b into jenkinsci:master Jul 2, 2016

oleg-nenashev added a commit that referenced this pull request Jul 3, 2016

Noting #2417, #2428, #2424, #2390, #2421, #2425, #2419, #1976

ab4b42b

samatdav pushed a commit to samatdav/jenkins that referenced this pull request Jul 25, 2016

Noting jenkinsci#2417, jenkinsci#2428, jenkinsci#2424, jenkinsci#2390, …

338bca6

…jenkinsci#2421, jenkinsci#2425, jenkinsci#2419, jenkinsci#1976

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-20187] - Handle growing files when creating a tar file. #2419

[JENKINS-20187] - Handle growing files when creating a tar file. #2419

alvarolobato commented Jun 22, 2016

ghost commented Jun 22, 2016

jglick Jun 22, 2016

alvarolobato Jun 22, 2016

jglick Jun 22, 2016

jtnord Jun 22, 2016 •

edited

jglick Jun 22, 2016

alvarolobato Jun 22, 2016

jglick commented Jun 22, 2016

oleg-nenashev commented Jun 22, 2016

alvarolobato commented Jun 22, 2016

jglick Jun 22, 2016

alvarolobato commented Jun 23, 2016

oleg-nenashev commented Jun 26, 2016

alvarolobato commented Jun 27, 2016

oleg-nenashev commented Jun 29, 2016

jglick Jun 29, 2016

jglick commented Jun 29, 2016

ghost commented Jun 29, 2016

oleg-nenashev commented Jun 30, 2016

alvarolobato commented Jun 30, 2016 via email

alvarolobato commented Jun 30, 2016

oleg-nenashev commented Jun 30, 2016

[JENKINS-20187] - Handle growing files when creating a tar file. #2419

[JENKINS-20187] - Handle growing files when creating a tar file. #2419

Conversation

alvarolobato commented Jun 22, 2016

ghost commented Jun 22, 2016

jglick Jun 22, 2016

Choose a reason for hiding this comment

alvarolobato Jun 22, 2016

Choose a reason for hiding this comment

jglick Jun 22, 2016

Choose a reason for hiding this comment

jtnord Jun 22, 2016 • edited

Choose a reason for hiding this comment

jglick Jun 22, 2016

Choose a reason for hiding this comment

alvarolobato Jun 22, 2016

Choose a reason for hiding this comment

jglick commented Jun 22, 2016

oleg-nenashev commented Jun 22, 2016

alvarolobato commented Jun 22, 2016

jglick Jun 22, 2016

Choose a reason for hiding this comment

alvarolobato commented Jun 23, 2016

oleg-nenashev commented Jun 26, 2016

alvarolobato commented Jun 27, 2016

oleg-nenashev commented Jun 29, 2016

jglick Jun 29, 2016

Choose a reason for hiding this comment

jglick commented Jun 29, 2016

ghost commented Jun 29, 2016

oleg-nenashev commented Jun 30, 2016

alvarolobato commented Jun 30, 2016 via email

alvarolobato commented Jun 30, 2016

oleg-nenashev commented Jun 30, 2016

jtnord Jun 22, 2016 •

edited