Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8231640: (prop) Canonical property storage #5372

Closed
wants to merge 28 commits into from
Closed
Changes from 2 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
85748cf
8231640: (prop) Canonical property storage
jaikiran Aug 29, 2021
641864e
use @implNote to explain the use of the environment variable
jaikiran Sep 7, 2021
64e2065
implement review suggestions:
jaikiran Sep 8, 2021
1ded17f
adjust testcases to verify the new semantics
jaikiran Sep 8, 2021
867ec99
update javadoc @implNote to match latest changes
jaikiran Sep 8, 2021
848ded8
Implement Stuart's suggestion on date format:
jaikiran Sep 9, 2021
5326b7c
update the new tests to match the new date format expectations and al…
jaikiran Sep 9, 2021
7736a8f
Implement Stuart's suggestion to use Collections instead of Arrays fo…
jaikiran Sep 9, 2021
c9d3cb8
update javadoc to match latest changes
jaikiran Sep 9, 2021
a29d0f0
dummy commit to trigger GitHub actions job to try and reproduce an un…
jaikiran Sep 9, 2021
a9b71d2
Implement Roger's suggestions:
jaikiran Sep 10, 2021
06ff3bd
jcheck fix - trailing whitespace in test
jaikiran Sep 10, 2021
1d24a3a
allow free-form text for java.util.Properties.storeDate system property
jaikiran Sep 10, 2021
c1dfb18
Update javadoc based on Stuart's inputs
jaikiran Sep 12, 2021
ff34ad5
Implement Roger's and the agreed upon decision to allow free-form tex…
jaikiran Sep 14, 2021
6447f9b
Merge latest from master branch
jaikiran Sep 14, 2021
9237466
unused imports
jaikiran Sep 14, 2021
315f3c8
update the javadoc to clarify how line terminator characters are hand…
jaikiran Sep 14, 2021
14711a9
Add a @implNote to specify the order in which the properties are writ…
jaikiran Sep 14, 2021
6f5f1be
Introduce a test to make sure backslash character in the system prope…
jaikiran Sep 14, 2021
7098a2c
Address review suggestions:
jaikiran Sep 15, 2021
79d1052
- Clarify how overriden Properties#entrySet() method impacts the orde…
jaikiran Sep 15, 2021
e2effb9
(Only doc/comment changes):
jaikiran Sep 16, 2021
eb31d28
- Implement Mark's suggestion in CSR, to rename java.util.Properties.…
jaikiran Sep 17, 2021
458c1fd
Implement Mark's suggestion in CSR to include the java.properties.dat…
jaikiran Sep 17, 2021
e350721
Roger's suggestion to reword the implSpec text
jaikiran Sep 18, 2021
ed5c221
reduce the line length of code comment
jaikiran Sep 22, 2021
944cbf1
Revert "Implement Mark's suggestion in CSR to include the java.proper…
jaikiran Sep 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -41,8 +41,13 @@
import java.nio.charset.Charset;
import java.nio.charset.IllegalCharsetNameException;
import java.nio.charset.UnsupportedCharsetException;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
import java.security.AccessController;
import java.security.PrivilegedAction;
import java.time.DateTimeException;
import java.time.Instant;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.util.concurrent.ConcurrentHashMap;
import java.util.function.BiConsumer;
import java.util.function.BiFunction;
@@ -167,10 +172,41 @@ public class Properties extends Hashtable<Object,Object> {
*/
private transient volatile ConcurrentHashMap<Object, Object> map;

// cached, string representation of any Date parsed out of the SOURCE_DATE_EPOCH environment variable
private static volatile String cachedDateComment;
// true if SOURCE_DATE_EPOCH environment variable value has been parsed
private static volatile boolean sourceDateEpochParsed;
@SuppressWarnings("removal")
private static final class LazyDateCommentProvider {
// formatter used while writing out current date. this formatter matches the format
// used by java.util.Date.toString()
private static final DateTimeFormatter currentDateFormatter =
DateTimeFormatter.ofPattern("EEE MMM dd HH:mm:ss zzz yyyy");
private static final String cachedDateComment;

static {
String sourceDateEpoch = System.getSecurityManager() == null
? System.getenv("SOURCE_DATE_EPOCH")
: AccessController.doPrivileged((PrivilegedAction<String>)
() -> System.getenv("SOURCE_DATE_EPOCH"));
Copy link
Contributor

@RogerRiggs RogerRiggs Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

It can be documented as part of the spec with the javadoc tag "{@systemProperty ...}"
There is a cache of properties in jdk.internal.util.StaticProperty that captures the state when the runtime is initialized. For specific properties, they are cached and available without regard to the setting of the SecurityManager or not. (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.)

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.

String dateComment = null;
if (sourceDateEpoch != null) {
try {
long epochSeconds = Long.parseLong(sourceDateEpoch);
dateComment = "#" + DateTimeFormatter.RFC_1123_DATE_TIME
.withLocale(Locale.ROOT)
.withZone(ZoneOffset.UTC)
.format(Instant.ofEpochSecond(epochSeconds));
} catch (NumberFormatException | DateTimeException e) {
// ignore any value that cannot be parsed for the SOURCE_DATE_EPOCH.
// store APIs will subsequently use current date, in their date comments
}
}
cachedDateComment = dateComment;
}

private static String getDateComment() {
return cachedDateComment != null
? cachedDateComment
: "#" + currentDateFormatter.format(ZonedDateTime.now());
}
}

/**
* Creates an empty property list with no default values.
@@ -920,9 +956,17 @@ private void store0(BufferedWriter bw, String comments, boolean escUnicode)
if (comments != null) {
writeComments(bw, comments);
}
writeDateComment(bw);
bw.write(LazyDateCommentProvider.getDateComment());
bw.newLine();
synchronized (this) {
for (Map.Entry<Object, Object> e : new TreeMap<>(map).entrySet()) {
var entries = map.entrySet().toArray(new Map.Entry<?, ?>[0]);
Arrays.sort(entries, new Comparator<Map.Entry<?, ?>>() {
Copy link
Member Author

@jaikiran jaikiran Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part here, intentionally doesn't use a lambda, since from what I remember seeing in some mail discussion, it was suggested that using lambda in core parts which get used very early during JVM boostrap, should be avoided. If that's not a concern here, do let me know and I can change it to a lambda.

Copy link
Member

@stuart-marks stuart-marks Sep 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fair concern, but writing out a properties file is a pretty high-level operation that doesn't seem likely to be used during bootstrap. Also, I observe that the doPrivileged block above uses a lambda. So I think we're probably ok to use a lambda here. But if you get an inexplicable error at build time or at startup time, this would be the reason why.

Assuming we're ok with lambdas, it might be easier to use collections instead of arrays in order to preserve generic types. Unfortunately the map is Map<Object, Object> so we have to do some fancy casting to get the right type. But then we can use Map.Entry.comparingByKey() as the comparator. (Note that this uses lambda internally.)

Something like this might work:

        @SuppressWarnings("unchecked")
        var entries = new ArrayList<>(((Map<String, String>)(Map)map).entrySet());
        entries.sort(Map.Entry.comparingByKey());

@Override
public int compare(Map.Entry<?, ?> o1, Map.Entry<?, ?> o2) {
return ((String) o1.getKey()).compareTo((String) o2.getKey());
}
});
for (Map.Entry<?, ?> e : entries) {
String key = (String)e.getKey();
String val = (String)e.getValue();
key = saveConvert(key, true, escUnicode);
@@ -937,59 +981,6 @@ private void store0(BufferedWriter bw, String comments, boolean escUnicode)
bw.flush();
}

private static void writeDateComment(BufferedWriter bw) throws IOException {
if (sourceDateEpochParsed && cachedDateComment == null) {
// SOURCE_DATE_EPOCH environment variable value has already been queried previously and
// the environment variable was either not set or its value couldn't be parsed to a Date.
// In either case, we write out the current date in the date comment
bw.write("#" + new Date());
bw.newLine();
return;
}
// Either the SOURCE_DATE_EPOCH environment variable needs to be queried or we have already
// queried it previously and are holding a cached string representation of that value.
// In either case, we first make sure the current caller has the necessary permissions to access
// that environment variable's value
String sourceDateEpoch = null;
try {
sourceDateEpoch = System.getenv("SOURCE_DATE_EPOCH");
} catch (SecurityException se) {
// caller code doesn't have permissions to SOURCE_DATE_EPOCH environment variable.
// Use current date in comment
bw.write("#" + new Date());
bw.newLine();
return;
}
// caller code has permissions to the environment variable, OK to use (any parseable) value
// of that environment variable in the date comment
if (!sourceDateEpochParsed) {
synchronized (Properties.class) {
if (!sourceDateEpochParsed) {
try {
String dateComment = null;
if (sourceDateEpoch != null) {
try {
Date d = new Date(Long.parseLong(sourceDateEpoch) * 1000);
// use the same format as that of Date.toGMTString() and a neutral locale for reproducibility
DateFormat df = new SimpleDateFormat("d MMM yyyy HH:mm:ss 'GMT'", Locale.ROOT);
df.setTimeZone(TimeZone.getTimeZone("GMT"));
dateComment = "#" + df.format(d);
} catch (NumberFormatException nfe) {
// ignore any value that cannot be parsed for the SOURCE_DATE_EPOCH.
// store APIs will subsequently use current date, in their date comments
}
}
cachedDateComment = dateComment;
} finally {
sourceDateEpochParsed = true;
}
}
}
}
bw.write(cachedDateComment != null ? cachedDateComment : "#" + new Date());
bw.newLine();
}

/**
* Loads all of the properties represented by the XML document on the
* specified input stream into this properties table.
@@ -28,16 +28,20 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.time.Duration;
import java.time.Instant;
import java.time.ZoneOffset;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Locale;
import java.util.Properties;
import java.util.TimeZone;

/*
* @test
@@ -49,15 +53,14 @@
public class StoreReproducibilityTest {

private static final String ENV_SOURCE_DATE_EPOCH = "SOURCE_DATE_EPOCH";
private static final String GMT_DATE_FORMAT = "d MMM yyyy HH:mm:ss 'GMT'";

public static void main(final String[] args) throws Exception {
// no security manager enabled
testWithoutSecurityManager();
// security manager enabled and security policy allows read permissions on getenv.SOURCE_DATE_EPOCH
testWithSecMgrCallerHasPermission();
// security manager enabled and security policy doesn't allow getenv.SOURCE_DATE_EPOCH permission
testWithSecMgrCallerNotPermitted();
// security manager enabled and security policy explicitly allows read permissions on getenv.SOURCE_DATE_EPOCH
testWithSecMgrExplicitPermission();
// security manager enabled and no explicit getenv.SOURCE_DATE_EPOCH permission
testWithSecMgrNoSpecificPermission();
// invalid/unparsable value for SOURCE_DATE_EPOCH
testInvalidSourceDateEpochValue();
}
@@ -98,17 +101,23 @@ private static void testWithoutSecurityManager() throws Exception {
* properties to a file. The launched Java program is passed an environment variable value for
* {@code SOURCE_DATE_EPOCH} environment variable and the date comment written out to the file
* is expected to use this value.
* The launched Java program is run with the default security manager and is granted
* a {@code read} permission on {@code getenv.SOURCE_DATE_EPOCH}.
* The program is launched multiple times with the same value for {@code SOURCE_DATE_EPOCH}
* and the output written out by each run of this program is verified to be exactly the same.
* Additionally, the date comment that's written out is verified to be the expected date that
* corresponds to the passed {@code SOURCE_DATE_EPOCH}.
* The launched Java program is run with the default security manager and the launched program that
* uses the Properties.store() APIs is granted a {@code read} permission on {@code getenv.SOURCE_DATE_EPOCH},
* thus allowing it to see the actual value of the environment variable.
*/
private static void testWithSecMgrCallerHasPermission() throws Exception {
final Path policyFile = Path.of(System.getProperty("test.src"),
"source-date-epoch-policy").toAbsolutePath();
private static void testWithSecMgrExplicitPermission() throws Exception {
final Path policyFile = Files.createTempFile("8231640", ".policy");
Files.write(policyFile, Collections.singleton("""
grant {
// test writes/stores to a file, so FilePermission
permission java.io.FilePermission "<<ALL FILES>>", "read,write";
// explicitly grant read on SOURCE_DATE_EPOCH to verifies store() APIs work fine
permission java.lang.RuntimePermission "getenv.SOURCE_DATE_EPOCH", "read";
};
"""));
final List<Path> storedFiles = new ArrayList<>();
final String sourceDateEpoch = "1234342423";
for (int i = 0; i < 5; i++) {
@@ -135,34 +144,42 @@ private static void testWithSecMgrCallerHasPermission() throws Exception {
* properties to a file. The launched Java program is passed an environment variable value for
* {@code SOURCE_DATE_EPOCH} environment variable and the date comment written out to the file
* is expected to use this value.
* The launched Java program is run with the default security manager. The launched program that
* uses the Properties.store() APIs is NOT granted any permission on {@code getenv.SOURCE_DATE_EPOCH}.
* It is expected and verified in this test that the absence of such a permission will cause the
* date comment to be the "current date" instead of the date corresponding to the {@code SOURCE_DATE_EPOCH}
* The launched Java program is run with the default security manager and is NOT granted
* any explicit permission for {@code getenv.SOURCE_DATE_EPOCH}.
* The program is launched multiple times with the same value for {@code SOURCE_DATE_EPOCH}
* and the output written out by each run of this program is verified to be exactly the same.
* Additionally, the date comment that's written out is verified to be the expected date that
* corresponds to the passed {@code SOURCE_DATE_EPOCH}.
*/
private static void testWithSecMgrCallerNotPermitted() throws Exception {
final Path policyFile = Path.of(System.getProperty("test.src"),
"source-date-epoch-policy-no-perm").toAbsolutePath();
private static void testWithSecMgrNoSpecificPermission() throws Exception {
final Path policyFile = Files.createTempFile("8231640", ".policy");
Files.write(policyFile, Collections.singleton("""
grant {
// test writes/stores to a file, so FilePermission
permission java.io.FilePermission "<<ALL FILES>>", "read,write";
// no other grants, not even "read" on SOURCE_DATE_EPOCH. test should still
// work fine and the date comment should correspond to the value of SOURCE_DATE_EPOCH
};
"""));
final List<Path> storedFiles = new ArrayList<>();
final String sourceDateEpoch = "1234342423";
for (int i = 0; i < 2; i++) {
for (int i = 0; i < 5; i++) {
final Path tmpFile = Files.createTempFile("8231640", ".props");
storedFiles.add(tmpFile);
final ProcessBuilder processBuilder = ProcessTools.createJavaProcessBuilder(
"-Djava.security.manager",
"-Djava.security.policy=" + policyFile.toString(),
StoreTest.class.getName(),
tmpFile.toString(),
i % 2 == 0 ? "--use-outputstream" : "--use-writer");
processBuilder.environment().put(ENV_SOURCE_DATE_EPOCH, sourceDateEpoch);
final Date processLaunchedAt = new Date();
// launch with a second delay so that we can then verify that the date comment
// written out by the program is "after" this date
Thread.sleep(1000);
executeJavaProcess(processBuilder);
assertExpectedSourceEpochDate(tmpFile, sourceDateEpoch);
if (!StoreTest.propsToStore.equals(loadProperties(tmpFile))) {
throw new RuntimeException("Unexpected properties stored in " + tmpFile);
}
assertCurrentDate(tmpFile, processLaunchedAt);
}
assertAllFileContentsAreSame(storedFiles, sourceDateEpoch);
}

/**
@@ -224,21 +241,22 @@ private static void assertExpectedSourceEpochDate(final Path destFile,
+ " when " + ENV_SOURCE_DATE_EPOCH + " was set " +
"(to " + sourceEpochDate + ")");
}
final Date parsedDate;
long parsedSecondsSinceEpoch;
try {
final DateFormat df = new SimpleDateFormat(GMT_DATE_FORMAT, Locale.ROOT);
df.setTimeZone(TimeZone.getTimeZone("GMT"));
parsedDate = df.parse(dateComment);
} catch (ParseException pe) {
var d = DateTimeFormatter.RFC_1123_DATE_TIME
.withLocale(Locale.ROOT)
.withZone(ZoneOffset.UTC).parse(dateComment);
parsedSecondsSinceEpoch = Duration.between(Instant.ofEpochSecond(0), Instant.from(d)).toSeconds();
} catch (DateTimeParseException pe) {
throw new RuntimeException("Unexpected date " + dateComment + " in stored properties " + destFile
+ " when " + ENV_SOURCE_DATE_EPOCH + " was set " +
"(to " + sourceEpochDate + ")");
"(to " + sourceEpochDate + ")", pe);

}
final long expected = Long.parseLong(sourceEpochDate) * 1000;
if (parsedDate.getTime() != expected) {
throw new RuntimeException("Expected " + expected + " millis since epoch but found "
+ parsedDate.getTime());
final long expected = Long.parseLong(sourceEpochDate);
if (parsedSecondsSinceEpoch != expected) {
throw new RuntimeException("Expected " + expected + " seconds since epoch but found "
+ parsedSecondsSinceEpoch);
}
}

This file was deleted.

This file was deleted.