New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hudson.Util improvements #3226

Merged
merged 15 commits into from Mar 3, 2018

Conversation

4 participants
@ghost

ghost commented Jan 10, 2018

Miscellaneous improvements to hudson.Util

Proposed changelog entries

None required

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@oleg-nenashev

@oleg-nenashev oleg-nenashev self-requested a review Jan 14, 2018

throw new IOException("Failed to create a new directory "+tmp);
return tmp;
// https://stackoverflow.com/questions/617414/how-to-create-a-temporary-directory-folder-in-java
return Files.createTempDirectory("jenkins").toFile();

This comment has been minimized.

@dwnusbaum

dwnusbaum Jan 24, 2018

Member

I believe that on POSIX systems Files.createTempDirectory will create a directory with 0700&(~umask) permissions while the previous code will create a directory with 0777&(~umask) permissions. Would be good to confirm to prevent another JENKINS-48407 .

This comment has been minimized.

@ghost

ghost Feb 25, 2018

Hello Devin,

I checked and you are correct; Files.createTempDirectory() does use permissions 0700&(~umask) for the tmp dir.

I found PR #3161 whose approach could be used here. I will add a commit to do this.

This comment has been minimized.

@ghost

ghost Feb 25, 2018

New commit added. Would you please take a look?

@Wadeck

Wadeck approved these changes Feb 20, 2018

🐝 only Devin's point to ensure but the rest LGTM

@@ -1579,7 +1558,10 @@ public static void closeAndLogFailures(@CheckForNull Closeable toClose, @Nonnull
try {
toClose.close();
} catch(IOException ex) {
logger.log(Level.WARNING, String.format("Failed to close %s of %s", closeableName, closeableOwner), ex);
LogRecord record = new LogRecord(Level.WARNING, "Failed to close {0} of {1}");

This comment has been minimized.

@daniel-beck

daniel-beck Feb 20, 2018

Member

4x the code for the frequent case when WARNING doesn't get logged? Really?

This comment has been minimized.

@ghost

ghost Feb 25, 2018

Should I revert this change?

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 24, 2018

Merge conflict, but seems to be almost ready to go. @dtrebbien would be great to get your response to the review comments

Daniel Trebbien added some commits Jan 10, 2018

Daniel Trebbien
Replace use of _ as an identifier
_ is a keyword in Java 9.
Daniel Trebbien
Copy to NULL_OUTPUT_STREAM
Also, closing source is moved to a finally block, to guarantee that it
will be closed per the documentation; it was technically possible for
the input stream to not be closed if construction of the MessageDigest,
DigestInputStream, or byte[] buffer failed for some reason (e.g. out of
memory).
Daniel Trebbien
Throw an IllegalArgumentException if data has odd length
Previously, a StringIndexOutOfBoundsException would be thrown, which is
less clear.
@ghost

This comment has been minimized.

ghost commented Feb 25, 2018

Rebased to resolve the merge conflict.

@ghost

This comment has been minimized.

ghost commented Feb 25, 2018

Note that the test failure, Windows / Windows Publishing / tail – hudson.util.TextFileTest, is being observed on other test runs. See, e.g.: #3219 (comment)

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Feb 25, 2018

yep :(

@dwnusbaum dwnusbaum self-requested a review Feb 26, 2018

@dwnusbaum

This comment has been minimized.

Member

dwnusbaum commented Feb 27, 2018

The test failure was fixed by #3310, so the build should pass if you merge with master.

@ghost

This comment has been minimized.

ghost commented Feb 28, 2018

Okay. Merge commit pushed.

// https://github.com/jenkinsci/jenkins/pull/3161 )
final Path tempPath;
final String tempDirNamePrefix = "jenkins";
if (FileSystems.getDefault().supportedFileAttributeViews().contains("posix")) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 28, 2018

Member

Maybe this actually needs a changelog

return toHexString(md5.digest());
} catch (NoSuchAlgorithmException e) {
throw new IOException("MD5 not installed",e); // impossible
} finally {
source.close();

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 28, 2018

Member

It is a bugfix, the behavior actually violates Javadoc. Could you please submit a JIRA ticket?
It seems @jglick removed that in the fix for https://issues.jenkins-ci.org/browse/JENKINS-18178 (see the commented code below). e0a3a1d

This comment has been minimized.

@dwnusbaum

dwnusbaum Feb 28, 2018

Member

The previous code used try-with-resources try (DigestInputStream in = new DigestInputStream(source, md5)), which guarantees the underlying stream is closed, right? e0a3a1d has a finally block which closes in, which itself will close source, and the code was later updated in 7b39424 to use try-with-resources to close in.

My only concern is if there is an issue here with only closing source and not in.

This comment has been minimized.

@ghost

ghost Feb 28, 2018

Yes, the closing of source was usually accomplished by closing the DigestInputStream in = new DigestInputStream(source, md5) resource (implicitly, as part of the try-with-resource's automatic finally block which would close the resource, which in turn would close the wrapped InputStream source).

I don't think that there is a problem with not closing the DigestInputStream. Examining the OpenJDK sources, DigestInputStream does not override close(). DigestInputStream extends FilterInputStream, whose close() implementation only closes the wrapped InputStream:

    public void close() throws IOException {
        in.close();
    }

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Mar 3, 2018

Member

right. Sorry, I should have read the code more carefully

@oleg-nenashev

The code itself LGTM

@@ -864,6 +863,8 @@ public static String toHexString(@Nonnull byte[] bytes) {
@Nonnull
public static byte[] fromHexString(@Nonnull String data) {
if (data.length() % 2 != 0)
throw new IllegalArgumentException("data must have an even number of hexadecimal digits");

This comment has been minimized.

@dwnusbaum

dwnusbaum Feb 28, 2018

Member

Does this need a changelog entry as well?

@ghost

This comment has been minimized.

ghost commented Feb 28, 2018

Here are suggested changelog entries:

  • Util.fromHexString() now throws an IllegalArgumentException if the string parameter does not have even-length.
  • Improve the implementation of Util.createTempDir().

@oleg-nenashev oleg-nenashev merged commit 8f51260 into jenkinsci:master Mar 3, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Mar 5, 2018

IllegalArgumentException replacing StringIndexOutOfBoundsException doesn't make the notability threshold.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment