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

[mandy chung]

Here is the updated webrev:
http://cr.openjdk.java.net/~mchung/jdk11/webrevs/8198249/webrev.04/

I added some comments to clarify the implementation.

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
>