RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

[Jason Mehrens]

The only other fix would be to collect all errors thrown (using addSuppressed) during close and throw only after the we have attempted to close all handlers.  Then we would retain the behavior of always throwing errors.

Jason

________________________________________
From: Daniel Fuchs <daniel.fuchs at oracle.com>
Sent: Tuesday, November 01, 2016 5:43 AM
To: David Holmes; core-libs-dev
Cc: Jason Mehrens
Subject: Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

On 28/10/16 20:32, David Holmes wrote:
> Hi Daniel,
>
> I've read the bug report on this and this issue "smells" to me.
> LinkageError should not be a special case IMHO. The existing code was
> trying to go the "ignore everything but 'serious errors' " route - but
> without really considering what constitutes a "serious error". I think
> the fact a LinkageError can turn up here reflects a bug in the code that
> throws it - or possibly in something in between.
>
> There should be a very clear exception handling policy for this code,
> and not special cases IMHO. Having that policy depend on whether
> shutdown is in progress is okay to me as we know that things can behave
> unexpectedly in that case. So I would advocate for a more general
>
> +             } catch (Throwable e) {
> +                 // Ignore all exceptions while shutting down
> +                 if (globalHandlersState != STATE_SHUTDOWN) {
> +                     throw e;
> +                 }
>

Hi David,

This is something I had been considering too.

In the end it made me feel uneasy to trap all errors - like OOME
or IllegalAccessError (swallowing IAE is not great when
one migrates from JDK 8 to JDK 9 where IAE helps diagnose
encapsulation issues) - so I opted for the minimal fix.

I agree that just trapping LinkageError is a bit strange,
but we do have a use case for that - which AFAIU is not
uncommon.

Anyway, if you ask me then the root of the problem is
that the various shutdown hook can run in parallel and in
unspecified order - so making the reset() more robust by
trapping all errors might not be such a bad idea after all...
Here is an updated webrev:

http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.02/


              } catch (Exception ex) {
                  // Problems closing a handler?  Keep going...
+            } catch (Error e) {
+                // ignore Errors while shutting down
+                if (globalHandlersState != STATE_SHUTDOWN) {
+                    throw e;
+                }
              }

best regards,

-- daniel


> Cheers,
> David
>
> On 28/10/2016 9:51 PM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a trivial  patch for:
>>
>> 8152515: (logging) LogManager.resetLogger should ignore LinkageError
>> https://bugs.openjdk.java.net/browse/JDK-8152515
>>
>>
>> Patch:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/
>>
>> The issue might occur at shutdown, when a handler that makes uses
>> of some APIs provided by an OSGI bundle which was already closed
>> by the shutdown process is in turn closed by the LogManager.Cleaner
>> thread. In that case some subclasses of LinkageError may be thrown,
>> interrupting the reset process and preventing other handlers from
>> being closed properly.
>>
>> The patch proposes to trivially ignore LinkageError at shutdown while
>> the LogManager.Cleaner thread is running.
>>
>> best regards,
>>
>> -- daniel