Remove CloseQuietly and migrate its usages to other methods. by gianm · Pull Request #10247 · apache/druid

gianm · 2020-08-06T09:43:14Z

CloseQuietly.close is a method that closes something. It suppresses and logs any
IOExceptions it gets, but re-throws any non-IOExceptions. It is used extensively and it
seems likely to me that most usages did not intend to have these specific behaviors.
So this patch gets rid of the method and migrates call sites to various more sanely
behaving methods.

These other methods include:

New method CloseableUtils.closeAndWrapExceptions, which wraps IOExceptions
in RuntimeExceptions for callers that just want to avoid dealing with
checked exceptions. Most usages were migrated to this method, because it
looks like they were mainly attempts to avoid declaring a throws clause,
and perhaps were unintentionally suppressing IOExceptions.
New method CloseableUtils.closeInCatch, designed to properly close something
in a catch block without losing exceptions. Some usages from catch blocks
were migrated here, when it seemed that they were intended to avoid checked
exception handling, and did not really intend to also suppress IOExceptions.
New method CloseableUtils.closeAndSuppressExceptions, which sends all
exceptions to a "chomper" that consumes them. Nothing is thrown or returned.
The behavior is slightly different: with this method, all exceptions are
suppressed, not just IOExceptions. Calls that seemed like they had good
reason to suppress exceptions were migrated here.
Some calls were migrated to try-with-resources, in cases where it appeared
that CloseQuietly was being used to avoid throwing an exception in a finally
block.

🎵 You don't have to go home, but you can't stay here... 🎵

These other methods include: 1) New method CloseableUtils.closeAndWrapExceptions, which wraps IOExceptions in RuntimeExceptions for callers that just want to avoid dealing with checked exceptions. Most usages were migrated to this method, because it looks like they were mainly attempts to avoid declaring a throws clause, and perhaps were unintentionally suppressing IOExceptions. 2) New method CloseableUtils.closeInCatch, designed to properly close something in a catch block without losing exceptions. Some usages from catch blocks were migrated here, when it seemed that they were intended to avoid checked exception handling, and did not really intend to also suppress IOExceptions. 3) New method CloseableUtils.closeAndSuppressExceptions, which sends all exceptions to a "chomper" that consumes them. Nothing is thrown or returned. The behavior is slightly different: with this method, _all_ exceptions are suppressed, not just IOExceptions. Calls that seemed like they had good reason to suppress exceptions were migrated here. 4) Some calls were migrated to try-with-resources, in cases where it appeared that CloseQuietly was being used to avoid throwing an exception in a finally block. 🎵 You don't have to go home, but you can't stay here... 🎵

clintropolis

Still working on review of this, a bit more than half done. I need to think harder about the places which were previously eating an IOException that will now have to deal with a RuntimeException, specifically the http client and json parser stuff, and also the low level segment mapping stuffs.

clintropolis · 2020-09-01T02:13:35Z

    Closer closer = Closer.create();
    closer.registerAll(cursors);
-    CloseQuietly.close(closer);
+    CloseableUtils.closeAndWrapExceptions(closer);


Looking at the usage of this method, it appears that it is expected to eat the exceptions:

catch (Throwable t) { closeAllCursors(sequenceCursors); cancellationGizmo.cancel(t); out.offer(ResultBatch.TERMINAL); }

so, I think this method should be using closeAndSuppressExceptions. I guess callers could also be re-arranged to do the things that absolutely need done before closing the cursors, however, nothing would directly catch an exception thrown here, so it is sort of like screaming into the void, so I think we should just suppress them.

I agree that it would be better to suppress IOException. We shouldn't throw IOException immediately especially at Line 650 since the terminal resultBatch must be added to the outputQueue after closing all cursors.

I'll change it to CloseableUtils.closeAndSuppressExceptions

clintropolis · 2020-09-01T02:30:33Z

        }
      }
-      CloseQuietly.close(columnarMultiInts);
+      CloseableUtils.closeAndWrapExceptions(columnarMultiInts);


Hmm, I know this is a test and probably doesn't really matter, but it seems like mapper.close should get called probably. Should this be using closeAndSuppressExceptions to ensure that it gets closed, or make them both part of a closer that gets passed into this method?

I put them both in a single CloseableUtils.closeAll

clintropolis · 2020-09-01T02:30:42Z

        }
      }
-      CloseQuietly.close(columnarMultiInts);
+      CloseableUtils.closeAndWrapExceptions(columnarMultiInts);


same comment about closing mapper

Same solution

clintropolis · 2020-09-01T02:35:36Z

    }
-    catch (Exception ex) {
-      CloseQuietly.close(closer);
-      return Optional.empty();


This method was using CloseQuietly to eat exceptions, although probably not well enough since it was only eating IOExceptions. I think it should probably be using closeAndSuppressExceptions.

Per the javadocs of acquireReferences:

... * IMPORTANT NOTE: to fulfill the contract of this method, implementors must return a closeable to indicate that the * reference can be acquired, even if there is nothing to close. Implementors should avoid allowing this method or the * {@link Closeable} it creates to throw exceptions. * * For callers: if this method returns non-empty, IT MUST BE CLOSED, else reference counts can potentially leak. ...

Good point, changed this to suppress.

jihoonson · 2020-09-03T05:36:07Z

    Closer closer = Closer.create();
    closer.registerAll(cursors);
-    CloseQuietly.close(closer);
+    CloseableUtils.closeAndWrapExceptions(closer);


I agree that it would be better to suppress IOException. We shouldn't throw IOException immediately especially at Line 650 since the terminal resultBatch must be added to the outputQueue after closing all cursors.

jihoonson · 2020-09-03T05:52:11Z

+
+    // Second exception
+    Assert.assertEquals(1, e.getSuppressed().length);
+    Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));


Hmm, why not IllegalArgumentException?

Ah, yeah, that's a better test. I changed it.

jihoonson · 2020-09-03T05:52:45Z

+
+    // Second exception
+    Assert.assertEquals(1, e.getSuppressed().length);
+    Assert.assertThat(e.getSuppressed()[0], CoreMatchers.instanceOf(RuntimeException.class));


Also changed this.

jihoonson · 2020-09-03T06:09:52Z

-    catch (Exception ex) {
-      CloseQuietly.close(resultSupplier);
-      throw ex;
+    catch (Throwable e) {


Why catching Throwables vs catching only Exceptions? It doesn't seem useful to do anything when you see any Error here.

Hmm, I can't think of a reason. Changed it back to Exception.

Now that I think about it more, I guess the reason is that we always want to close stuff, even if some weird Throwable got thrown. It's making it behave like a finally or like a try-with-resources. I changed it back to Throwable. Let me know if you disagree.

jihoonson · 2020-09-03T06:13:41Z

    public void close()
    {
-      CloseQuietly.close(closer);
+      CloseableUtils.closeAndWrapExceptions(closer);


nit: you can modify the signature of this method to throw IOExceptoin, but this should also be fine.

The superclass won't allow it. (It overrides close and declares it to throw nothing.) I guess I could change the superclass, but I didn't want to go that far.

jihoonson · 2020-09-03T06:23:16Z

    protected void loadBuffer(int bufferNum)
    {
-      CloseQuietly.close(holder);
+      CloseableUtils.closeAndWrapExceptions(holder);


This seems fine because ResourceHolder.close() doesn't throw IOException, but this code seems confusing to me since, until looking at the code closely, I wasn't sure if it's fine to not execute the below lines when this line throws an exception. How about adding a comment here, such as "this line should never throw any checked exceptions"?

Same comment for other places where close ResourceHolders.

I didn't realize it didn't throw IOException. So actually, I could just remove the CloseableUtils call completely here, and replace it with holder.close(). I'll do that.

Did that, along with the other block layout column suppliers.

jihoonson · 2020-09-03T06:28:58Z


        if (prevVal instanceof Closeable) {
-          CloseQuietly.close((Closeable) prevVal);
+          CloseableUtils.closeAndWrapExceptions((Closeable) prevVal);


This changes the behavior, but any callers of fromIterableVersionOne() doesn't seem creating GenericIndexed from Closeable types. How about removing this?

If it's ok with you, I'd prefer to leave it in for this patch, because I didn't want to go through and make sure it's always ok to skip closing.

stale · 2020-11-05T02:27:21Z

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

stale · 2020-12-25T13:53:08Z

This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

gianm · 2021-01-08T02:27:56Z

nice try, stalebot

stale · 2021-01-08T02:27:58Z

This issue is no longer marked as stale.

stale · 2021-01-08T02:27:59Z

This pull request/issue is no longer marked as stale.

gianm · 2021-01-08T17:55:43Z

Fixed up some merge conflicts.

gianm · 2021-03-13T02:59:53Z

Fixed up some more merge conflicts.

clintropolis

🤘

clintropolis · 2021-03-16T23:01:27Z

+   * Like {@link Closeable#close()} but sends any exceptions to the provided Consumer, and then throws them away.
+   *
+   * If the Consumer throws an exception, that exception is thrown by this method. So if your intent is to chomp
+   * exceptions, you should avoid writing a Consumer that might thrown an exception.


Suggested change

* exceptions, you should avoid writing a Consumer that might thrown an exception.

* exceptions, you should avoid writing a Consumer that might throw an exception.

gianm · 2021-10-22T20:12:45Z

Fixed up some more merge conflicts.

gianm added 5 commits August 6, 2020 02:40

Remove unused import.

8dad2b7

Fix up various issues.

69f3e84

Merge branch 'master' into fix-closequietlys

2233672

Adjustments to tests.

d9f3820

jihoonson added the Refactoring label Aug 13, 2020

jihoonson mentioned this pull request Aug 13, 2020

Segment backed broadcast join IndexedTable #10224

Merged

8 tasks

gianm added 5 commits August 13, 2020 16:59

Merge branch 'master' into fix-closequietlys

91e5ada

Fix null handling.

13b10f1

Merge branch 'master' into fix-closequietlys

686ee12

Merge branch 'master' into fix-closequietlys

b5a2b89

Additional test.

1fa8c4b

clintropolis reviewed Sep 1, 2020

View reviewed changes

jihoonson reviewed Sep 3, 2020

View reviewed changes

stale Bot added the stale label Nov 5, 2020

stale Bot closed this Dec 25, 2020

gianm reopened this Jan 8, 2021

stale Bot removed the stale label Jan 8, 2021

gianm added 3 commits January 7, 2021 22:30

Merge branch 'master' into fix-closequietlys

319e7c1

Adjustments from review.

45e7fb1

Merge branch 'master' into fix-closequietlys

fa0a0e8

gianm added 3 commits January 13, 2021 17:42

Merge branch 'master' into fix-closequietlys

5d97b00

Fixup style stuff.

0272ab5

Merge branch 'master' into fix-closequietlys

0ac8ca8

gianm added 2 commits January 14, 2021 12:22

Fix NPE caused by holder starting out null.

113d622

Merge branch 'master' into fix-closequietlys

f158f9d

clintropolis approved these changes Mar 17, 2021

View reviewed changes

Merge branch 'master' into fix-closequietlys

a22190e

gianm added 3 commits October 22, 2021 13:13

Fix spelling.

c81478b

Merge branch 'master' into fix-closequietlys

fe7edcf

Chomp Throwables too.

762a88f

clintropolis merged commit 98ecbb2 into apache:master Oct 24, 2021

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

gianm deleted the fix-closequietlys branch September 23, 2022 19:23

	* exceptions, you should avoid writing a Consumer that might thrown an exception.
	* exceptions, you should avoid writing a Consumer that might throw an exception.

Conversation

gianm commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis left a comment

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!

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!

stale Bot commented Nov 5, 2020

Uh oh!

stale Bot commented Dec 25, 2020

Uh oh!

gianm commented Jan 8, 2021

Uh oh!

stale Bot commented Jan 8, 2021

Uh oh!

stale Bot commented Jan 8, 2021

Uh oh!

gianm commented Jan 8, 2021

Uh oh!

gianm commented Mar 13, 2021

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm commented Oct 22, 2021

Uh oh!

Reviewers

Assignees

gianm commented Aug 6, 2020 •

edited

Loading