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