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

[David Holmes]

Hi Daniel,

This change seems fine to me. Given there is an inadequate specification 
for the public reset() method in regards to possible exceptions 
occurring whilst it attempts all its task, I think this is as far as we 
should go at this time.

Thanks,
David

On 1/11/2016 8:43 PM, Daniel Fuchs wrote:
> 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
>