RFR: 8231640: (prop) Canonical property storage [v9]

Fri Sep 10 14:47:51 UTC 2021

On Fri, 10 Sep 2021 10:15:45 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> The commit in this PR implements the proposal for enhancement that was discussed in the core-libs-dev mailing list recently[1], for https://bugs.openjdk.java.net/browse/JDK-8231640
>> 
>> At a high level - the `store()` APIs in `Properties` have been modified to now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env variable is set, then instead of writing out the current date time as a date comment, the `store()` APIs instead will use the value set for this env variable to parse it to a `Date` and write out the string form of such a date. The implementation here uses the `d MMM yyyy HH:mm:ss 'GMT'` date format and `Locale.ROOT` to format and write out such a date. This should provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, intentionally, no changes in the date format of the "current date" have been done.
>> 
>> These  modified `store()` APIs work in the presence of the `SecurityManager` too. The caller is expected to have a read permission on the `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that permission, then the implementation of these `store()` APIs will write out the "current date" and will ignore any value that has been set for the `SOURCE_DATE_EPOCH` env variable. This should allow for backward compatibility of existing applications, where, when they run under a `SecurityManager` and perhaps with an existing restrictive policy file, the presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` APIs.
>> 
>> The modified `store()` APIs will also ignore any value for `SOURCE_DATE_EPOCH` that  cannot be parsed to an `long` value. In such cases, the `store()` APIs will write out the "current date" and ignore the value set for this environment variable. No exceptions will be thrown for such invalid values. This is an additional backward compatibility precaution to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking applications.
>> 
>> An additional change in the implementation of these `store()` APIs and unrelated to the date comment, is that these APIs will now write out the property keys in a deterministic order. The keys will be written out in the natural ordering as specified by `java.lang.String#compareTo()` API.
>> 
>> The combination of the ordering of the property keys when written out and the usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment should together allow for reproducibility of the output generated by these `store()` APIs.
>> 
>> New jtreg test classes have been introduced to verify these changes. The primary focus of `PropertiesStoreTest` is the ordering aspects of the property keys that are written out. On the other hand `StoreReproducibilityTest` focuses on the reproducibility of the output generated by these APIs.  The `StoreReproducibilityTest` runs these tests both in the presence and absence of `SecurityManager`. Plus, in the presence of SecurityManager, it tests both the scenarios where the caller is granted the requisite permission and in other case not granted that permission.
>> 
>> These new tests and existing tests under `test/jdk/java/util/Properties/` pass with these changes.
>> 
>> [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html
>> [2] https://reproducible-builds.org/specs/source-date-epoch/
>
> Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision:
> 
>   jcheck fix - trailing whitespace in test

Hi Jaikiran,

On 9/9/21 9:59 PM, mlbridge[bot] wrote:
>
> /Mailing list message from Jaikiran Pai 
> ***@***.***> on core-libs-dev 
> ***@***.***>:/
>
> Hello Roger,
>
> On 10/09/21 12:32 am, Roger Riggs wrote:
>
>     src/java.base/share/classes/java/util/Properties.java line 187:
>
>         185: ? System.getenv("SOURCE_DATE_EPOCH")
>         186: : AccessController.doPrivileged((PrivilegedAction<String>)
>         187: () -> System.getenv("SOURCE_DATE_EPOCH"));
>         The format of SOURCE_DATE_EPOCH is fine, but referring to an
>         external document as part of the OpenJDK spec is undesirable
>         in the long run. Previous comments gave support to using the
>         environment variable directly but
>         it very unlike similar cases that use system properties when
>         the behavior of an API needs to be overridden or modified.
>
>     As a system property, specified on the command line, it would be
>     visible when the program is invoked,
>     explicitly intended to change behavior and not having a context
>     sensitive effect. Named perhaps, java.util.Properties.storeDate.
>
> Would this system property have a value that represents what
> SOURCE_DATE_EPOCH environment variable would have contained? i.e. it is
> the "A UNIX timestamp, defined as the number of seconds, excluding leap
> seconds, since 01 Jan 1970 00:00:00 UTC."? So users can potentially do
> -Djava.util.Properties.storeDate=${SOURCE_DATE_EPOCH}?
>
> Or would this system property be just some kind flag, which when having
> a value of "true" would expect the java.util.Properties code to lookup
> the SOURCE_DATE_EPOCH environment variable and use that value from the
> environment variable?
>
> I'm guessing it's the former where the value is the number of epoch
> seconds, but just wanted to be sure before I do this change.
>
I agree with Magnus' suggestion to make it just a comment string to be 
included if it is non-null and non-empty.
>
>     (BTW, there is no need to special case doPriv calls, they are
>     pretty well optimized when the security manager is not set and it
>     keeps the code simpler.)
>
> From what I see in the implementation of
> AccessController.doPriveleged(), I see that it does a bunch of
> uncoditional work (like Reflection.getCallerClass() and then
> Reference.reachabilityFence() calls). Do you mean that additional work
> is neglible in the absence of a security manager, that this special
> casing of the doPriv call can be removed?
>
(The code is gone now)
I meant that the testing for SM == null to avoid the doPriv won't 
improve performance much and performance isn't critical here.
The context will be null, getCallerClass is optimized(an Intrinsic), and 
the reachabilityFence() is effectively a no-op, to keep gc from 
reclaiming objects too soon.
So all those a lightweight or optimized away.

>     Given the low frequency of calls to store(), even caching the
>     parsed date doesn't save much.
>     And the cost is the cost of the size and loading an extra class.
>
> Sounds fine. I'll remove the caching part in my subsequent update.
>

Thanks, Roger

> -Jaikiran
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub 
> <https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/5372*issuecomment-916571385__;Iw!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsILYLySc$>, 
> or unsubscribe 
> <https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AAIRSVEFZBJXHRXMASLW4MDUBFRA7ANCNFSM5DNMPLNA__;!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsHmTJQki$>.
> Triage notifications on the go with GitHub Mobile for iOS 
> <https://urldefense.com/v3/__https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675__;!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsIOavqZC$> 
> or Android 
> <https://urldefense.com/v3/__https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign*3Dnotification-email*26utm_medium*3Demail*26utm_source*3Dgithub__;JSUlJSU!!ACWV5N9M2RV99hQ!Yuv9mklDfoz0-neVqyTkIVkT2pStt3pp56LC_GtNhZ1XaO6Iq34i47TUsHjk8HBC$>. 
>
>

-------------

PR: https://git.openjdk.java.net/jdk/pull/5372