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

Refactor TextFile.lines() #3211

Merged
merged 5 commits into from Mar 7, 2018

Conversation

2 participants
@ghost

ghost commented Dec 28, 2017

Introduces a LinesStream utility class that is responsible for closing
the underlying file handle.

Switches to calling Files.newBufferedReader() instead of wrapping the
InputStream returned by Files.newInputStream() in a BufferedReader of
an InputStreamReader.

Creates the file reader at the time that lines() is invoked, instead of
at the time that iterator() of the returned Iterable is invoked. This
can help prevent confusion as to the cause of an exception, whether it's
because of a problem opening the file or reading from it.

Proposed changelog entries

  • RFE: Developer: Introduce new jenkins.util.io.LinesStream class for iterating through files without fully caching them in memory

Maybe also:

  • RFE: Correctly propagate exceptions when reading Slave2Master security configuration file

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

InvalidPathException is not handled anymore

}
}
};
public @Nonnull LinesStream lines() throws IOException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 29, 2017

Member

The exception needs to be documented at least

This comment has been minimized.

@ghost
* {@link #iterator()} to obtain a second or subsequent <code>Iterator</code>
* throws <code>IllegalStateException</code>.
*/
public class LinesStream implements Closeable, Iterable<String> {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 29, 2017

Member

@since TODO or make it @Restricted if there is no plan to use it externally

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 29, 2017

Member

Maybe you want to create a new utility class and pass file as a constructor parameter. There are other use-cases for such class, so no need to keep it nested

This comment has been minimized.

@ghost

ghost Dec 29, 2017

I extracted the class to top-level and added @since TODO.

private transient Iterator<String> iterator;
public LinesStream() throws IOException {
in = Files.newBufferedReader(file.toPath()); // uses UTF-8 by default

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 29, 2017

Member

🐛 It does not longer handle InvalidPathException. Use hudson.Util.fileToPath()

This comment has been minimized.

@ghost
@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 6, 2018

@dtrebbien we do not get notifications when somebody does a force push. And generally there is no need to force push, we can squash commits during the merge if needed

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

@ghost

This comment has been minimized.

ghost commented Jan 6, 2018

Oh, sorry.

To summarize what I've done via force-pushing:

  • I addressed your requested changes from the first revision of the patch.
  • I relocated the new LinesStream utility class to the jenkins.util.io package.
  • I modified the documentation of TextFile.lines() to mention that the caller must close the returned LinesStream.
Daniel Trebbien
Refactor TextFile.lines()
Introduces a LinesStream utility class that is responsible for closing
the underlying file handle.

Switches to calling Files.newBufferedReader() instead of wrapping the
InputStream returned by Files.newInputStream() in a BufferedReader of
an InputStreamReader.

Creates the file reader at the time that lines() is invoked, instead of
at the time that iterator() of the returned Iterable is invoked. This
can help prevent confusion as to the cause of an exception, whether it's
because of a problem opening the file or reading from it.
@ghost

This comment has been minimized.

ghost commented Jan 8, 2018

Rebased to resolve conflict.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Jan 18, 2018

Sorry, I will review it eventually. JEP-200 and GSoC soak all my time this month

@@ -26,15 +27,17 @@ public ConfigFile(File file) {
protected abstract COL create();
protected abstract COL readOnly(COL base);
public synchronized void load() {
public synchronized void load() throws IOException {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 3, 2018

Member

It breaks the source compatibility IIUC. I would rather introduce a new one and deprecate load()

This comment has been minimized.

@daniel-beck

daniel-beck Feb 4, 2018

Member

Why do we care about source compatibility?

This comment has been minimized.

@oleg-nenashev
try {
load();
} catch (IOException e) {
throw new RuntimeException(e);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 3, 2018

Member

It would be rather preferable to create a new method with error propagation

}
return r;
} catch (IOException e) {
throw new RuntimeException(e);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 3, 2018

Member

Should be at least documented in the iterator Javadoc

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 3, 2018

Member

OTOH it does not make it worse being compared to the original code

* @since TODO
*/
@CleanupObligation
public class LinesStream implements Closeable, Iterable<String> {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 3, 2018

Member

Maybe @Restricted(NoExternalUse.class)? No strong opinion. Runtimized IOException on read error should be documented if it goes to public API

@oleg-nenashev

@dtrebbien it looks good to me excepting one minor source compatibility thing. Would it be possible to address this comment so that I merge it?

@ghost

This comment has been minimized.

ghost commented Mar 5, 2018

I have now added ConfigFile.load2() and deprecated the original ConfigFile.load() method.

@oleg-nenashev oleg-nenashev merged commit 145c606 into jenkinsci:master Mar 7, 2018

1 check passed

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

oleg-nenashev added a commit to oleg-nenashev/jenkins that referenced this pull request Mar 10, 2018

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Mar 11, 2018

Added the changelog entries

oleg-nenashev added a commit that referenced this pull request Mar 11, 2018

Amends #3211: Restore Source compatibility of hudson.util.TextFile#li…
…nes() (#3340)

* Amends #3211: Restore Source compatibility of hudson.util.TextFile#lines()

* Address comments from @dtrebbien
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment