Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

Hello Stuart,

On 03/07/19 8:57 AM, Stuart Marks wrote:
> Hi Jaikiran,
>
> OK, good analysis. Rather a lot of issues for what seems like a simple
> patch, eh?

Indeed, but I kind of expected that :)


>
>  - 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.

Thank you. Although not for end(), I have now used this @implSpec in the
first version of my patch[1].


>
>  - Relationship between end() and close()
>
> I think close() should have more-or-less the same semantics as end(),
> namely, pretty much the way end() is specified now regarding releasing
> resources. But it should be allowable to call both in either order.
>
>     try (var inf = new Inflater()) {
>     ...
>     # explicitly call end()
>     inf.end();
>     }
>
> I think this should allowed, but it shouldn't be required. The
> application can call either close() or end() and this will have the
> effect of releasing the native resources.
>
Agreed.


> A key question is whether close() should be specified to call end() --
> which is subject to being overridden by suclasses -- or whether
> close() is defined to do the same thing as the end() implementation in
> this class does. This can be implemented by taking the body of the
> current end() method and refactoring it into an internal method and
> then having both close() and end() call that internal method.
>
> If close() calls end() then AC+TWR might call close() twice, and
> therefore call end() twice, which might be a problem for subclasses.
> So to be really conservative we might want to do the internal-method
> refactoring to avoid this problem.
>
> On the other hand, suppose a subclass has extra resources it frees in
> its end() method, which then calls super.end(). Even though this class
> would inherit AC, using it in TWR would be a *bug* because close()
> would call the internal method to free the superclass resources, but
> this would leak the subclass resources. That sounds like a mistake to
> perpetuate in the API.
>
> It's possible for a subclass to override end() in such a way that's
> not idempotent, but I think this is unlikely. I'm leaning toward
> risking the small possibility of incompatibility in declaring end()
> idempotent, allowing close() simply to call end(). This makes TWR more
> useful in the long run, which seems like a better position to be in.
> Of course, if somebody turns up evidence that this would break a bunch
> of stuff, we should reconsider.
>
I thought about this a bit more and I think we should call end() from
close() instead of neither doing whatever end() is doing nor extracting
out the logic into a private method. The advantage of close() calling
end() is, (as you state), it allows the subclasses to correctly cleanup
any resources those subclasses are holding on to. Furthermore, I think
we can still make it such that the (potentially overridden) end()
doesn't get called more than once even if close() gets called multiple
times.

In the patch[1], I've implemented and documented close() such that it
calls end() atmost once.


>
>  - Deprecation of end()
>
> I don't think deprecation of end() is useful. It'll just cause noise
> for people. Uses such as
>
>     var deflater = new Deflater();
>     try {
>     ...
>     } finally {
>     deflater.end();
>     }
>
> are valid and correct and needn't be changed (though using TWR is
> preferable, this is more of a style issue).

OK and I do agree about the noise the deprecation can cause in this case.


>
>  - Undefined behavior after close()/end()
>
> OK, I'll admit this is possibly out of scope, but the line in the
> specs about "once end() is called, the behavior is undefined" rubs me
> the wrong way. Right now, most operations seem to call ensureOpen(),
> which throws NPE if the object has been closed. But "undefined" allows
> anything to happen, including filling the output buffer with garbage.
> That seems wrong. We might not want to specify what exception is
> thrown, but we might want to specify that *AN* exception is thrown --
> which must be a RuntimeExcep

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

[Jaikiran Pai]

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 > > 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

Re: RFR: 8224560: (tz) Upgrade time-zone data to tzdata2019a and 8225580: tzdata2018i integration causes test failures on jdk-13

[Andrew John Hughes]

On 05/07/2019 15:16, Ramanand Patil wrote:
> Hi all,
> Please review the patch for tzdata2019a integration into jdk project.
> Webrev: http://cr.openjdk.java.net/~rpatil/8224560/webrev.00/
> Bugs: https://bugs.openjdk.java.net/browse/JDK-8224560
> https://bugs.openjdk.java.net/browse/JDK-8225580
> 
> Summary:
> - The fix contains cumulative tzdata changes from tzdata2018i and 
> tzdata2019a, as tzdata2018i was not integrated into jdk/jdk earlier.
> - In JDK-13/14, multiple failures were seen during integration of tzdata2018i 
> (JDK-8225580), those are fixed now. Many thanks to Naoto for providing a fix 
> for this in CLDRConverter.java.

I would guess this is due to the CLDR update (JDK-8221432: Upgrade CLDR
to Version 35.1) in OpenJDK 13, making TimeZone.getAvailableIDs
inappropriate in some way?

Fix looks good. One minor change:

+AVAILABLE_TZIDS = new HashSet(ZoneId.getAvailableZoneIds());

is missing the  or <>:

+AVAILABLE_TZIDS = new HashSet<>(ZoneId.getAvailableZoneIds());

Will this fix also resolve JDK-8225580? If so, it's probably worth
mentioning both bug IDs in the commit.

> - There are 2 type of test failures in TestZoneInfo310.java file, which are 
> solved in this patch by providing workarounds, But a permanent fix needs to 
> be added in future for the same. Below are the 2 bugs created to track the 
> development on it:
>   1. https://bugs.openjdk.java.net/browse/JDK-8223388: 
> TestZoneInfo310.java fails post tzdata2018i integration:
> This failure is seen for the TimeZones which are having zone rules defined 
> till year 2037 or beyond. For now, the failing zones are skipped.
> The supporting test class ZoneInfo.java has maxYear defined 
> http://hg.openjdk.java.net/jdk/jdk/file/d01b345865d7/test/jdk/sun/util/calendar/zi/Zoneinfo.java#l40,
>  changing this max value greater than the zone rule's last year also fixes 
> the issue, but further investigation is needed on why this boundary condition 
> is affecting the test behavior.

I wonder if 2037 is in someway related to the rollover of 32-bit time
values?

>   2. JDK-8227293: https://bugs.openjdk.java.net/browse/JDK-8227293 
> TestZoneInfo310.java fails post tzdata2019a integration for Palestine zone 
> rules:
> There are many hacks and assumptions in the class 
> sun.util.calendar.ZoneInfoFile.java. This issue looks because of the code 
> starting from here: 
> http://hg.openjdk.java.net/jdk/jdk/file/963924f1c891/src/java.base/share/classes/sun/util/calendar/ZoneInfoFile.java#l552
> There is an assumption where the transition date is >=24,(line 577 and 599) 
> it assumes it is the "last" rule, but it is not last rule in case of 
> Asia/Gaza and Asia/Hebron zones.
> For now, I have fixed these 2 problematic timezones by overwriting the 
> assumption made on line 577, where date of month dom = startRule.dom;
> I think, overriding of the second jdk hack on line 599 is not required as the 
> "dom" is calculated from the last rule there. Keeping this bug open as we 
> need to find a generic solution for this issue, without hard-coding the 
> values and adding specific time zone names in exclusion as seen in many 
> places in this class file.
> 
> - The patch has passed all the related testing including JCK tests.
> 
> 
> Regards,
> Ramanand.
> 
> 
> 
> 
> 

Looks good to me, with the above minor adjustment.

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222
https://keybase.io/gnu_andrew