RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Jeroen Frijters]

David Daney wrote:
> This is the new version of my HTTP patch.  It keeps promoting (near) 
> silence from the approvers, and I keep finding and fixing new bugs. 
> Also it has been about a week and I fixed another bug, so I thought I 
> would post the current version.

Thanks for this rewrite. I reviewed the patch and it looks good. I have
two small nitpicks:

+    if (-1 == r)

In Java this isn't really needed (as you can't accidentally do an assign
here anyway), so it is uncommon to put the constant first.

LimitedLengthInputStream shouldn't have a finalize().

> OK to commit?

I vote yes.

Regards,
Jeroen

Re: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[David Daney]

Jeroen Frijters wrote:
> David Daney wrote:
> 
> LimitedLengthInputStream shouldn't have a finalize().
> 

Let's consider the case where a client program did not read the entire 
body of the response:

As implemented in the patch, the finalize is indeed needed to clean up 
the mess and return the connection to the connection pool.

However I have been going back and forth on this matter, and now am of 
the opinion that if a client does not read the entire body that the 
connection should just be abandoned and not returned to the pool.  Think 
of the case where you only read a little bit from the head of a very 
large response body.  In this case do you want the runtime to read and 
throw away the rest of the body (which could have unbounded size) just 
so it can reuse the connection?  I am starting to think not.

Opinions welcome, but I think I might get rid of the code that handles 
this case.

> 
>>OK to commit?
> 
> 
> I vote yes.

Thanks for the vote, but what I really am looking for is official 
permission.

David Daney

Re: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Tom Tromey]

>>>>> "Jeroen" == Jeroen Frijters <jeroen@sumatra.nl> writes:

>> OK to commit?

Jeroen> I vote yes.

Me too.  Please go ahead and check it in.
Thanks for doing this.

Tom

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Boehm, Hans]

> -----Original Message-----
> From: Jeroen Frijters
> 
> > In the example in your blog, the finalize method should be 
> > synchronized (or start with synchronized(this) {} ) to guarantee 
> > reachability of the object while one of the other 
> synchronized methods 
> > is running.
> 
> Huh? finalize calls the synchronized close method, so it 
> doesn't need to be synchronized.
Right.  I misread that.

> 
> > If you follow the (admittedly baroque) rules, there are 
> safe ways to 
> > clean up Java resources with finalizers as well, though clearly not 
> > when timing matters.
> > 
> > And you actually need to follow very similar rules for 
> java.lang.ref.
> 
> Well at the very least the fact that you control the 
> threading with a reference queue makes things a little 
> easier. For example, the I/O blocking issue is not a problem 
> if you can do it in your own thread.
> 
I agree.  It makes this sort of thing easier, and it partially dodges
some of the finalizer ordering issues, at the expense of often needing a
new thread to process the queue.  It doesn't help with the "premature"
finalization issue.

Hans

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Jeroen Frijters]

Boehm, Hans wrote:
> Blocking on IO indeed seems dubious.  But finalizers almost 
> always need to acquire at least one lock.

Agreed, but outside of the lock needed to protect the finalizable
resource it's probably best to avoid taking other locks, because it is
very easy to introduce potential deadlocks that way.

> In the example in your blog, the finalize method should be
> synchronized (or start with synchronized(this) {} ) to
> guarantee reachability of the object while one of the other 
> synchronized methods is running.

Huh? finalize calls the synchronized close method, so it doesn't need to
be synchronized.

> If you follow the (admittedly baroque) rules, there are safe ways to
> clean up Java resources with finalizers as well, though 
> clearly not when timing matters.
> 
> And you actually need to follow very similar rules for java.lang.ref.

Well at the very least the fact that you control the threading with a
reference queue makes things a little easier. For example, the I/O
blocking issue is not a problem if you can do it in your own thread.

Regards,
Jeroen

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Boehm, Hans]

> -----Original Message-----
> From:  Jeroen Frijters
> Finalizers are anything but simple.
That much we agree on.  And I agree with most of the advice in your
blog.

> They really should only 
> be used to free native resources (as a last resort), not for 
> any type of Java clean up. Blocking and locking should be 
> avoided in a finalizer. Typically there is only one finalizer 
> thread and you don't want to hog that by blocking on I/O.
Blocking on IO indeed seems dubious.  But finalizers almost always need
to acquire at least one lock.  In the example in your blog, the finalize
method should be synchronized (or start with synchronized(this) {} ) to
guarantee reachability of the object while one of the other synchronized
methods is running.

If you follow the (admittedly baroque) rules, there are safe ways to
clean up Java resources with finalizers as well, though clearly not when
timing matters.

And you actually need to follow very similar rules for java.lang.ref.

For details see my JavaOne talk.  (A version of the slides, with a
different set of
typos and logos, can also be found at
http://www.hpl.hp.com/personal/Hans_Boehm/misc_slides/java_finalizers.pd
f .)

Hans

> 
> I wrote about how finalize should be used on my blog: 
>
http://weblog.ikvm.net/permalink.aspx?guid=3E0ABC11-F486-4366-9127-CF38B
B6C3EF1

Regards,
Jeroen

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Jeroen Frijters]

David Daney wrote:
> A finalizer is simple and gets the job done.

Finalizers are anything but simple. They really should only be used to
free native resources (as a last resort), not for any type of Java clean
up. Blocking and locking should be avoided in a finalizer. Typically
there is only one finalizer thread and you don't want to hog that by
blocking on I/O.

I wrote about how finalize should be used on my blog:
http://weblog.ikvm.net/permalink.aspx?guid=3E0ABC11-F486-4366-9127-CF38B
B6C3EF1

Regards,
Jeroen

Re: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[David Daney]

Jeroen Frijters wrote:
> David Daney wrote:
> 
>>Jeroen Frijters wrote:
>>
>>>David Daney wrote:
>>>
>>>LimitedLengthInputStream shouldn't have a finalize().
>>
>>Let's consider the case where a client program did not read 
>>the entire body of the response:
>>
>>As implemented in the patch, the finalize is indeed needed to 
>>clean up the mess and return the connection to the connection
>>pool.
> 
> 
> I understand that was the motivation, but I just don't agree with it.
> Even *if* you wanted to do this, you should use a PhantomReference to
> keep track of the lifetime of the LimitedLengthInputStream instead of a
> finalize method.
> 

The LimitedLengthInputStream *is* the class that needs to do the 
cleanup.  Once a PhontomReference is enqueued the object is gone.  A 
finalizer is simple and gets the job done.

But this is all moot as I am going to kill it anyhow.

David Daney.

RE: [cp-patches] [PATCH] Fix PR classpath/24086, PR classpath/24091, PR classpath/24104 et al. ...

[Jeroen Frijters]

David Daney wrote:
> Jeroen Frijters wrote:
> > David Daney wrote:
> > 
> > LimitedLengthInputStream shouldn't have a finalize().
> 
> Let's consider the case where a client program did not read 
> the entire body of the response:
> 
> As implemented in the patch, the finalize is indeed needed to 
> clean up the mess and return the connection to the connection
> pool.

I understand that was the motivation, but I just don't agree with it.
Even *if* you wanted to do this, you should use a PhantomReference to
keep track of the lifetime of the LimitedLengthInputStream instead of a
finalize method.

> However I have been going back and forth on this matter, and 
> now am of the opinion that if a client does not read the entire body
> that the connection should just be abandoned and not returned to the 
> pool.  Think of the case where you only read a little bit from the
> head of a very large response body.  In this case do you want the
> runtime to read and throw away the rest of the body (which could
> have unbounded size) just so it can reuse the connection? 
> I am starting to think not.

Exactly. Another issue is that finalize (or the PhantomReference
solution) introduces non-determinisme that IMO simply does nobody any
favors.

> > I vote yes.
> 
> Thanks for the vote, but what I really am looking for is official 
> permission.

There is no official owner. If nobody complains in a couple of days,
just go ahead and commit. As long as you don't commit too close to a
snapshot release date, any left over issues will get worked out. In any
case, I believe your implementation is a big step forward.

Regards,
Jeroen