[11] RFR JDK-8198249: Remove deprecated Runtime::runFinalizersOnExit and System::runFinalizersOnExit

[David Holmes]

Hi Mandy,

tl;dr: I think this is now good to go.

On 22/02/2018 5:58 AM, mandy chung wrote:
> Here is the updated webrev:
> http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.04/
> 
> I added some comments to clarify the implementation.

src/java.base/share/classes/java/lang/Shutdown.java

  96                 if (VM.isShutdown()|| slot <= currentRunningHook)

Nit: need space after ()


  113     private static void runHooks() {
  114         synchronized (lock) {
  115             /* Guard against the possibility of a daemon thread 
invoking exit
  116              * after DestroyJavaVM initiates the shutdown sequence
  117              */
  118             if (VM.isShutdown()) return;
  119         }

I think this is actually impossible to hit, but that's a separate cleanup.

  139         // set shutdown state
  140         VM.shutdown();

Just an observation that we consider "shutdown" to be after all system 
shutdown hooks are run. So this (contrary to what I wrote in the CSR) 
narrows the window in which a concurrent exit(-1) would trigger an 
immediate halt rather than a hang. Which in turn strengthens the 
argument for just dropping that behaviour.

---

src/java.base/share/classes/jdk/internal/misc/VM.java

  103     public static void shutdown() {
  104         initLevel(SYSTEM_SHUTDOWN);
  105     }

Doing this is exactly what I meant by my previous comments.

Thanks,
David


> Mandy
> 
> On 2/21/18 11:34 AM, mandy chung wrote:
>> Hi David,
>>
>> I think I'm clear on the implementation now (my mistake that I 
>> neglected ApplicationShutdownHooks).  Shutdown class keeps the "system 
>> shutdown hooks".  ApplicationShutdownHooks is one system hook that 
>> starts all application shutdown hooks and waits until they finish.   
>> java.io.Console and DeleteOnExitHook are two other system shutdown 
>> hooks to restore console echo state or support deleteOnExit.
>>
>> The system shutdown hooks are all run in the thread initiating the 
>> shutdown that holds the Shutdown.class.
>>
>> On 2/20/18 10:27 PM, David Holmes wrote:
>>> Hi Mandy,
>>>
>>> On 21/02/2018 5:57 AM, mandy chung wrote:
>>>> Hi David,
>>>>
>>>> I reworked the change in Shutdown class and uses 
>>>> jdk.internal.misc.VM to maintain the shutdown state, either in 
>>>> progress or shutdown (i.e. all shutdown hooks have been started).
>>>>
>>>> What do you think this revised version:
>>>> http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.03/
>>>
>>> It confuses me a bit. On the one hand I wasn't sure why we needed to 
>>> bring the VM class into this, then on the other hand it perhaps made 
>>> sense to have VM track shutdown states as well as initialization states.
>>>
>>
>> The latter is my motivation and I think it's a good cleanup to have VM 
>> state to indicate it's shutdown.
>>
>>> But I'm not sure about the two-phase "shutdown" state - I think I'd 
>>> probably prefer a "shutdown initiated" state and a "shutdown 
>>> complete" state. Then no need to duplicate the constant values for 
>>> IN_PROGRESS and SHUTDOWN (which now need to be kept in sync with 
>>> changes to the VM class). It would also simplify the shutdown() logic.
>>>
>>
>> Shutdown::add method can be used to register a system hook while 
>> shutdown is in progress.  Actually currentRunningHook can be used to 
>> guard if Shutdown::add should reject to add a new hook.  I updated the 
>> webrev and no longer needs the two phases.
>>
>>> Also shutdown(level) should be using initLevel(value) so that it fits 
>>> in with the existing awaitInitLevel() logic and locking strategy! 
>>> Someone may want to wait for shutdown in the future. That also deals 
>>> with the locking issue in the Shutdown class because you don't need 
>>> to use "synchronized(lock)" because runHooks is only called within 
>>> "synchronized(Shutdown.class)". [To be fair the existing locking 
>>> strategy seems confused to me as well - or at least it confuses me.]
>>
>> I agree but I tried not to touch the existing locking strategy as it 
>> should be a separate changeset.  I prefer to add VM::shutdown method
>>>
>>> I also now realize that the changes I suggested for the Runtime.exit 
>>> docs was incorrect. The existing docs state that we only halt if 
>>> hooks have already been run - which corresponds to the old and new 
>>> code. I, for some reason that escapes me, claimed we only cared if 
>>> the hooks had been started, not completed - but that is inconsistent 
>>> with old spec and the implementation.
>>>
>>
>> I confused myself too.  A small change will fix it:
>>
>> 87 * <p> If this method is invoked after all shutdown hooks have already
>> 88 * been run and the status is nonzero then this method halts the
>> 89 * virtual machine with the given status code. Otherwise, this method
>>
>>
>>
>> Mandy
>>
>>> So apologies but what started out as a seemingly simple code removal, 
>>> has become a lot more complicated, and confusing to me.
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>>> On 2/15/18 9:14 PM, David Holmes wrote:
>>>>>
>>>>> All other updates seem okay. I did have one further thought - in 
>>>>> Runtime does this change:
>>>>>
>>>>>       public void runFinalization() {
>>>>> ! SharedSecrets.getJavaLangRefAccess().runFinalization();
>>>>>       }
>>>>>
>>>>> affect the classloading/initialization order at all?
>>>>
>>>> Runtime::runFinalization is not called by the system code.  So it 
>>>> won't be invoked during startup and hence won't change the 
>>>> classloading order during startup.
>>>>
>>>> Mandy
>>
>