AVRO-1985: Unreleased resources by gszadovszky · Pull Request #177 · apache/avro

gszadovszky · 2016-12-29T11:35:27Z

Fixes for some unreleased resource issues found by static code analyzer tool.

busbey · 2017-01-24T16:42:49Z

lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

+    } finally {
+      if (reader != null)
+        reader.close();
+      in.close();


Why the change to close the passed SeekableInput? Shouldn't that be up to the caller?

You're right, it should be closed by the caller but it is already implemented this way as we are closing the reader which closes the SeekableInput as well. I did not want to break backward compatibility but fix the actual situation that the resources are not closed every time.
That's why I've tried to highlight in the method comment that this method closes the specified SeekableInput.

Can we update DataFileReader to properly not close the passed input stream instead?

We can but it would be a backward incompatible change as this method is public.
If we would like to follow the usual pattern for this case we would need another method which does not close it and deprecate this one then remove this one in the next major release.
Or is it enough marking this JIRA with "incompatible change" and change as you are suggesting?

busbey · 2017-01-24T16:46:29Z

lang/java/tools/src/main/java/org/apache/avro/tool/IdlTool.java

-    if (args.size() == 2 && ! "-".equals(args.get(1))) {
-      parseOut = new PrintStream(new FileOutputStream(args.get(1)));
-    }
+      if (args.size() == 2 && !"-".equals(args.get(1))) {


it looks like these indents are incorrect.

Seems you're right. I'll check it out.

busbey · 2017-01-24T16:47:43Z

lang/java/tools/src/main/java/org/apache/avro/tool/Main.java

+      for (int i = in.read(buffer); i != -1; i = in.read(buffer))
+        System.err.write(buffer, 0, i);
+    } finally {
+      in.close();


Why are we closing in within this method instead of having the caller do it?

This one is a private method and it seemed to be easier and shorter to close the stream inside this method instead of the public caller. Do you think I should rewrite it?

yeah please have the caller close it instead. we should be internally disciplined.

Fair enough. On the way...

gszadovszky · 2017-02-07T13:53:28Z

@busbey, could you check my answers, please?

busbey · 2017-02-07T14:38:21Z

lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

+    } finally {
+      if (reader != null)
+        reader.close();
+      in.close();


Can we update DataFileReader to properly not close the passed input stream instead?

busbey · 2017-02-07T14:39:14Z

lang/java/tools/src/main/java/org/apache/avro/tool/Main.java

+      for (int i = in.read(buffer); i != -1; i = in.read(buffer))
+        System.err.write(buffer, 0, i);
+    } finally {
+      in.close();


yeah please have the caller close it instead. we should be internally disciplined.

gszadovszky · 2017-05-17T14:07:10Z

@busbey, could you please check the latest changes?

zivanfi · 2017-06-09T15:20:22Z

lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

-    return appendTo(new SeekableFileInput(file),
-                    new SyncableFileOutputStream(file, true));
+    SeekableInput input = new SeekableFileInput(file);
+    OutputStream output = new SyncableFileOutputStream(file, true);


I think this line should be inside the try block as well to guarantee that input is always closed. Or, alternatively, it may be before the first line if order does not matter.

zivanfi · 2017-06-09T15:21:21Z

lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

+    try {
+      return appendTo(input, output);
+    } finally {
+      input.close();


Could you please add a comment about why output does not have to be closed?

good point. Will do it.

zivanfi · 2017-06-09T15:22:30Z

lang/java/avro/src/main/java/org/apache/avro/file/DataFileWriter.java

  }

  /** Open a writer appending to an existing file.
+   * <strong>Since 1.9.0 this method does not close in.</strong>


1.9.0 is already out. Additionally, isn't this an API breaking change?

Yes, unfortunately, this changes didn't make 1.9.0. And yes, this is an API breaking change so the next target is 1.10.0. Will rewrite the comment.

1.9.0 isn't out yet afaik, so this should be fine as is.

Ah, sorry, I was thinking of Parquet... :(

Yes, that was my mistake too :)

zivanfi · 2017-06-09T15:30:07Z

lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java

+        if (fw != null)
+          fw.close();
+        if (fos != null)
+          fos.close();


It seems to me that the second close() may not be called if the first one throws an IOException.

fos.close() is invoked separately to close the file in case of the fw creation fails (fw starts writing at the constructor). If both fos and fw creation was successful than a problem occurs during writing fw.close() shall close the file. If it fails, fos.close() would probably fail too.

nandorKollar · 2017-06-09T15:28:58Z

lang/java/avro/src/main/java/org/apache/avro/data/Json.java

@@ -55,8 +56,12 @@ private Json() {}                               // singleton: no public ctor
  public static final Schema SCHEMA;
  static {
    try {


Do we have to support Java 1.6 in Avro, or we require 1.7+? I'm wondering if we can use try-with-resource statements instead of closing the stream in finally. Also, can we merge the two nested try{} blocks into one?

Currently, Avro supports 1.6 so we are not able to use try-with-resources, unfortunately.

We could start discussing dropping 1.6 support in the 1.9.0 release on the dev list.

gszadovszky · 2017-06-13T07:25:07Z

Pushed another update (hopefully, the last one).
@busbey, could you please check if this one suits you, so I can commit it.
Thanks a lot in advance.

busbey

please be sure to squash into a single commit with an appropriate commit message. otherwise LGTM.

Gabor Szadovszky and others added 2 commits December 29, 2016 12:27

Unreleased resource fixes

37cc158

Some modifications according to zi's findings

b2879f3

gszadovszky changed the title ~~Unreleased resource fixes~~ AVRO-1983: Unreleased resources Jan 13, 2017

gszadovszky changed the title ~~AVRO-1983: Unreleased resources~~ AVRO-1985: Unreleased resources Jan 18, 2017

busbey requested changes Jan 24, 2017

View reviewed changes

Indention correction

73711e6

busbey requested changes Feb 7, 2017

View reviewed changes

gszadovszky force-pushed the fortify-fixes branch from c020505 to 73711e6 Compare February 8, 2017 09:52

Updates according to busbey's comments

c5c728d

zivanfi reviewed Jun 9, 2017

View reviewed changes

nandorKollar reviewed Jun 9, 2017

View reviewed changes

Some additional modifications according to zi's comments

ec266fb

busbey approved these changes Jun 15, 2017

View reviewed changes

asfgit closed this in 528a1c8 Jun 16, 2017

Conversation

gszadovszky commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gszadovszky commented Jun 13, 2017

Uh oh!

busbey left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants