Hello Lance, On 03/07/19 11:27 PM, Lance Andersen wrote: > Just a thought below to consider or ignore ;-) >> On Jul 2, 2019, at 11:27 PM, Stuart Marks <stuart.ma...@oracle.com >> <mailto:stuart.ma...@oracle.com>> wrote: >> >> Hi Jaikiran, >> >> OK, good analysis. Rather a lot of issues for what seems like a >> simple patch, eh? >> >> - Idempotency >> >> Yes, it looks to me like Inflater.end() and Deflater.end() >> implementations are idempotent. As you point out, overrides by >> subclasses might not be. We should be clear when we're talking about >> the specific implementatations of the end() methods in the Deflater >> and Inflater classes, or whether we're talking about the contracts >> defined by these method specifications on these classes and subtypes. >> >> The behavior of an implementation in a class can be specified with >> @implSpec without imposing this on the contract of subclasses. This >> is useful for subclasses that need to know the exact behavior of >> calling super.end(), and also for callers who know they have an >> instance of a particular class and not a subclass. >> >> The upshot is that while we might not have the end() method's >> contract specify idempotency, we can certainly say so in an >> @implSpec, if doing this will help. I'm not sure we'll actually need >> it in this case, but let's keep it in mind. >> >> - Subclasses >> >> I don't think there are any subclasses in the JDK, and I did some >> searching and I didn't find any subclasses "in the wild" either. I >> did find a few uses of these classes though. If these classes are >> rarely subclassed, we might be able to get away with changing the >> contract, though this does entail some risk. > > > we could consider leaving end() as is and just introduce close() > which while it duplicates what end() does, it reduces the chance of > any potential complaints. > The issue with that is for subclasses which have their own resource cleanup code solely in their end() method. This will actually be the case for any subclasses that are out there currently. When we introduce close() API and users of Deflater/Inflater start using it, if we duplicate the logic of end() into our close(), then (as noted by Stuart too) the end() method of the subclasses will never get called and will lead to a resource leak of any resources those subclasses are holding onto.
>> >> >> - Deprecation of end() > > I don’t think we need to deprecate, just add a note to its javadoc > encouraging the use of close() going forward... Agreed and done in the initial version of the proposed patch http://cr.openjdk.java.net/~jpai/webrev/8225763/1/webrev/index.html -Jaikiran
