Re: [jdk17] RFR: 8268349: Provide more detail in JEP 411 warning messages [v2]
> More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. > > This is new PR for the `openjdk/jdk17` repo copied from > https://github.com/openjdk/jdk/pull/4400. A new commit is added. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: update warnings to match the new CSR - Changes: - all: https://git.openjdk.java.net/jdk17/pull/13/files - new: https://git.openjdk.java.net/jdk17/pull/13/files/a8fc259e..c62dff99 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=13=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=13=00-01 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk17/pull/13.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13 PR: https://git.openjdk.java.net/jdk17/pull/13
[jdk17] RFR: 8268349: Provide more detail in JEP 411 warning messages
More loudly and precise warning messages when a security manager is either enabled at startup or installed at runtime. This is new PR for the `openjdk/jdk17` repo copied from https://github.com/openjdk/jdk/pull/4400. A new commit is added. - Commit messages: - no cache, new warning, enhance one test, fix one test - 8268349: Provide more detail in JEP 411 warning messages Changes: https://git.openjdk.java.net/jdk17/pull/13/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=13=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268349 Stats: 105 lines in 6 files changed: 64 ins; 19 del; 22 mod Patch: https://git.openjdk.java.net/jdk17/pull/13.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13 PR: https://git.openjdk.java.net/jdk17/pull/13
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
I can re-license some code that decorates Concurrent collections with references, so you can do this without blocking. https://pfirmstone.github.io/JGDMS/jgdms-collections/apidocs/index.html On 9/06/2021 4:31 am, Alan Bateman wrote: On Tue, 8 Jun 2021 18:22:55 GMT, Weijun Wang wrote: I thought about that but not sure of performance impact. Is the worst problem that more than one warnings will be printed for a single caller? It's not really harmless. As for the frame, if the warning message only contain the caller class name and its code source, why is it worth using a key of multiple frames? The message will look the same. WeakHashMap access needs synchronization. Whether we need to cache to avoid excessive warnings isn't clear. If the SM is enabled once and never disabled/re-enabled then caching isn't interesting. On the other hand if there are programs that are enabling/disabling to execute subsets of code then maybe it is. Maybe we should just drop this and see if there is any feedback on the repeated warning? Not sure what you meant by "WeakHashMap access synchronization", it's just a noun without any other parts. Do you think synchronization is necessary? For the cache, I'm OK to drop it at the moment. I think it would be simpler to start out without the caller cache. Sorry the sentence got garbled, I was trying to repeat what I said above that WeakHashMap is not synchronized so you would need to add synchronization to use it. - PR: https://git.openjdk.java.net/jdk/pull/4400 -- Regards, Peter Firmstone 0498 286 363 Zeus Project Services Pty Ltd.
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 18:22:55 GMT, Weijun Wang wrote: >>> I thought about that but not sure of performance impact. Is the worst >>> problem that more than one warnings will be printed for a single caller? >>> It's not really harmless. >>> >>> As for the frame, if the warning message only contain the caller class name >>> and its code source, why is it worth using a key of multiple frames? The >>> message will look the same. >> >> WeakHashMap access needs synchronization. Whether we need to cache to avoid >> excessive warnings isn't clear. If the SM is enabled once and never >> disabled/re-enabled then caching isn't interesting. On the other hand if >> there are programs that are enabling/disabling to execute subsets of code >> then maybe it is. Maybe we should just drop this and see if there is any >> feedback on the repeated warning? > > Not sure what you meant by "WeakHashMap access synchronization", it's just a > noun without any other parts. Do you think synchronization is necessary? > > For the cache, I'm OK to drop it at the moment. I think it would be simpler to start out without the caller cache. Sorry the sentence got garbled, I was trying to repeat what I said above that WeakHashMap is not synchronized so you would need to add synchronization to use it. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 17:15:29 GMT, Alan Bateman wrote: >> I thought about that but not sure of performance impact. Is the worst >> problem that more than one warnings will be printed for a single caller? >> It's not really harmless. >> >> As for the frame, if the warning message only contain the caller class name >> and its code source, why is it worth using a key of multiple frames? The >> message will look the same. > >> I thought about that but not sure of performance impact. Is the worst >> problem that more than one warnings will be printed for a single caller? >> It's not really harmless. >> >> As for the frame, if the warning message only contain the caller class name >> and its code source, why is it worth using a key of multiple frames? The >> message will look the same. > > WeakHashMap access needs synchronization. Whether we need to cache to avoid > excessive warnings isn't clear. If the SM is enabled once and never > disabled/re-enabled then caching isn't interesting. On the other hand if > there are programs that are enabling/disabling to execute subsets of code > then maybe it is. Maybe we should just drop this and see if there is any > feedback on the repeated warning? Not sure what you meant by "WeakHashMap access synchronization", it's just a noun without any other parts. Do you think synchronization is necessary? For the cache, I'm OK to drop it at the moment. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 12:22:52 GMT, Weijun Wang wrote: > I thought about that but not sure of performance impact. Is the worst problem > that more than one warnings will be printed for a single caller? It's not > really harmless. > > As for the frame, if the warning message only contain the caller class name > and its code source, why is it worth using a key of multiple frames? The > message will look the same. WeakHashMap access synchronization. Whether we need to cache to avoid excessive warnings isn't clear. If the SM is enabled once and never disabled/re-enabled then caching isn't interesting. On the other hand if there are programs that are enabling/disabling to execute subsets of code then maybe it is. Maybe we should just drop this and see if there is any feedback on the repeated warning? - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 06:11:17 GMT, Alan Bateman wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. > > src/java.base/share/classes/java/lang/System.java line 331: > >> 329: >> 330: // Remember original System.err. setSecurityManager() warning goes >> here >> 331: private static PrintStream oldErrStream = null; > > I assume this should needs to be volatile and @Stable. I think we need a > better name for it too. Will add the modifiers. How about "originalErr"? > src/java.base/share/classes/java/lang/System.java line 336: > >> 334: // Remember callers of setSecurityManager() here so that warning >> 335: // is only printed once for each different caller >> 336: final static Map callersOfSSM = new >> WeakHashMap<>(); > > You can't use a WeakHashMap without synchronization but a big question here > is whether a single caller frame is sufficient. If I were doing this then I > think I would capture the hash of a number of stack frames to create a better > filter. I thought about that but not sure of performance impact. Is the worst problem that more than one warnings will be printed for a single caller? It's not really harmless. As for the frame, if the warning message only contain the caller class name and its code source, why is it worth using a key of multiple frames? The message will look the same. > src/java.base/share/classes/java/lang/System.java line 2219: > >> 2217: WARNING: java.lang.SecurityManager is >> deprecated and will be removed in a future release >> 2218: WARNING: -Djava.security.manager=%s >> will have no effect when java.lang.SecurityManager is removed >> 2219: """, smProp); > > Raw strings may be useful here but means the lines length are inconsistent > and makes it too hard to look at side by side diffs now. I understand what you mean when I switch to Split View. While I can extract the lines to a method, I somehow think it's not worth doing because for each type of warning the method is only called once. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 06:41:11 GMT, Alan Bateman wrote: > > You might want to make your "WARNING" consistent with the VM's "Warning" so > > that OutputAnalyzer's logic to ignore warnings will automatically ignore > > these too. > > The uppercase "WARNING" is intentional here, it was the same with illegal > reflective access warnings. I'm sure Max has or will run all tests to see if > there are any issues. Will definitely run all from tier1-tier9. I ran them multiple times while implementing JEP 411. I've seen warnings with "VM" word in the prefix and test methods that filter them out, but feel the warnings here are not related to VM. The new warnings do have impacts on some tests and I'll be very carefully not break them. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang wrote: > More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. Changes to LoggerFinderLoaderTest look reasonable to me. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Tue, 8 Jun 2021 06:30:00 GMT, David Holmes wrote: > You might want to make your "WARNING" consistent with the VM's "Warning" so > that OutputAnalyzer's logic to ignore warnings will automatically ignore > these too. The uppercase "WARNING" is intentional here, it was the same with illegal reflective access warnings. I'm sure Max has or will run all tests to see if there are any issues. - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang wrote: > More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. There are a number of hotspot tests that will trigger this warning, so please ensure they work correctly with the extra output. You might want to make your "WARNING" consistent with the VM's "Warning" so that OutputAnalyzer's logic to ignore warnings will automatically ignore these too. Thanks, David - PR: https://git.openjdk.java.net/jdk/pull/4400
Re: RFR: 8268349: Provide more detail in JEP 411 warning messages
On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang wrote: > More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. Changes requested by alanb (Reviewer). src/java.base/share/classes/java/lang/System.java line 331: > 329: > 330: // Remember original System.err. setSecurityManager() warning goes > here > 331: private static PrintStream oldErrStream = null; I assume this should needs to be volatile and @Stable. I think we need a better name for it too. src/java.base/share/classes/java/lang/System.java line 336: > 334: // Remember callers of setSecurityManager() here so that warning > 335: // is only printed once for each different caller > 336: final static Map callersOfSSM = new > WeakHashMap<>(); You can't use a WeakHashMap without synchronization but a big question here is whether a single caller frame is sufficient. If I were doing this then I think I would capture the hash of a number of stack frames to create a better filter. src/java.base/share/classes/java/lang/System.java line 2219: > 2217: WARNING: java.lang.SecurityManager is > deprecated and will be removed in a future release > 2218: WARNING: -Djava.security.manager=%s > will have no effect when java.lang.SecurityManager is removed > 2219: """, smProp); Raw strings may be useful here but means the lines length are inconsistent and makes it too hard to look at side by side diffs now. - PR: https://git.openjdk.java.net/jdk/pull/4400
