Hi Peter,
Thanks for the review and catching that.
Will fix.
Roger
On 1/6/2016 2:02 PM, Peter Levart wrote:
On 01/05/2016 10:16 PM, Roger Riggs wrote:
Hi Daniel,
webrev updated to revert changes to MeteredStream as too risky.
http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html
Hi Roger,
I briefly skimmed over the webrev, and found the following issue in
ProcessImpl:
420 // Register a cleaning function to close the handle
421 CleanerFactory.getCleaner().register(this, () ->
closeHandle(handle));
422
... 'handle' is an instance variable, which means that 'this' is
captured by the lambda. You will have to assign handle to a local var
and capture it instead to prevent 'this' to be captured.
Regards, Peter
On 1/5/2016 1:45 PM, Daniel Fuchs wrote:
Hi Roger,
Some early feedback:
I see that prior to your changes, MeteredStream.close() would
be called by finalize.
This will no longer be the case after
http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html
In the case where finalize would call close(), there can be no
queuedForCleanup activity
because there can be no strong reference from the
KeepAliveStreamCleaner thread
or the queue that holds the pending cleanups.
It cannot be in the Cache of connections being kept alive or the
thread that keeps them alive.
It might be possible that the underlying connection is still open;
but there is no advantage
of trying to drain it. There is the possibility of resurrecting it
by virtue of deciding that
it should be queued for cleanup and starting a new thread to do the
cleanup.
All in all, there is a risk of misunderstanding the dynamic behavior
and it would be safer
to leave this using finalize.
I see that MeteredStream has a subclass (KeepAliveStream) that
overrides close() - so KeepAliveStream probably requires a cleanup
as well - doesn't it?
Finally I'd suggest making the variable 'pi' final - since you're
passing that to the lambda you don't want to allow its value to
be modified afterwards (fortunately it's not - and I believe it
should be good to ensure it won't be).
ok, it would not go unnoticed, the compiler should complain if the
variable is no effectively final.
Thanks, Roger
I haven't looked at the other classes yet...
best regards,
-- daniel
On 05/01/16 19:24, Roger Riggs wrote:
The follow on work to adding the Cleaner is to replace uses of
finalization with uses of the Cleaner.
For the 'easy' cases in the java.base module, it is useful to
introduce
a private Cleaner for the
java.base module. It is proposed to be held weakly, to allow it to
terminate on a lightly loaded
system.
Webrev for Review:
http://cr.openjdk.java.net/~rriggs/webrev-cleaning-factory-8146028/
The 2nd step is using the Cleaner.
- Empty finalize methods should (I think) be removed; but since they
are part of the public spec
the process needs two full releases; so the proposal is to
deprecate
them first.
(The JEP 277 necessary changes will be updated when JEP 277
semantics are finalized)
- In a few cases, the code in the finalizer is redundant and if
removed
would allow
an optimization of the handling of the finalizer and future removal
of the finalize method.
Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-cleaning-finalizers/index.html
Thanks for comments and suggestions, Roger
