Bug ID: JDK-8195974 Replace use of java.util.logging in javafx with System logger

Other
openjfx11Fixed

I filed JDK-8209036 as a follow-on issue to use PlatformLogger instead of j.u.l in the systemTests.
06-08-2018
Changeset: 328f6b32bf0c Author: nlisker Date: 2018-08-06 11:47 -0700 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/328f6b32bf0c 8195974: Replace use of java.util.logging in javafx with System logger Reviewed-by: kcr, aghaisas
06-08-2018
Yes. I can do it his afternoon.
06-08-2018
So can this be pushed?
06-08-2018
I don't have Webkit built so I can't run it either.
06-08-2018
webrev.01 looks good to me. +1. Only thing that I did not perform was running of 'gradle :web:test' with this patch. If you can confirm this test run succeeds, your patch is good to be pushed. Once again, thanks for the exhaustive logger replacement work !!!
06-08-2018
I can confirm that gradle :web:test works fine. I ran the following: gradle -PFULL_TEST=true -PUSE_ROBOT=true test
06-08-2018
There are no more references to j.u.l in the FX runtime (i.e., in any of the javafx.* modules). However, there are a few tests in the systemTests project, which are in tests/system/, that use j.u.l.
03-08-2018
Didn't we get rid of j.u.l with this patch? I removed java.logging from web's and fxml's module-info.java.
03-08-2018
> I note that you could have avoided changing log(Level, msg, ...) to {warning,fine,finer}(msg, ...) by implementing the former method in PlatformLogger, but since you've already done it, there is no reason to go back. Yeah. When I started working on this I tried not to touch PlatformLogger because there were those exceptional cases (DRT etc.) and I didn't know how it would pan out, and I also wanted to remain flexible with JDK-8200236. After the smoke cleared these batch method changes were already there so I didn't bother with rolling them back. With an IDE it's relatively easy to transition between them anyway.
03-08-2018
Once Ajit finishes his review, this can be pushed.
03-08-2018
I would very much welcome getting rid of j.u.l dependency in a follow up task.
03-08-2018
The changes look fine to me, except for trailing white space that needs to be fixed in one file: modules/javafx.base/src/main/java/com/sun/javafx/binding/Logging.java I note that you could have avoided changing log(Level, msg, ...) to {warning,fine,finer}(msg, ...) by implementing the former method in PlatformLogger, but since you've already done it, there is no reason to go back. I ran a full set of tests, and also verified that JDK-8198470 is fixed by this (so can be closed as a duplicate). We might want to file a follow-on issue to implement Handler support in PlatformLogger, which would allow the systemTests that use java.util.logging to switch to the PlatformLogger. I don't know how much work that would be. If it is worth doing, then we could use it for the javafx.base tests rather than the tactical solution that was done (for good reasons) here. If this is too much work then we will need to keep the dependncy on j.u.l. Going down this path would mean that we would not ever drop PlatformLogger (e.g., as part of JDK-8200236). +1 pending the trailing whitespace fix (no need for a new webrev...this can be fixed by whoever pushes it).
03-08-2018
> Anyway, these different ErrorLoggers will use same System.Logger internally - so on a second thought, I think it is OK to have the implementation where ErrorLogger is kept via a singleton instance. Yes, this is what I was thinking. New patch: http://cr.openjdk.java.net/~nlisker/8195974/webrev.01/ Implemented the changes for ErrorLoggingUtility and the tests that use it as described in 3.4 and 3.5 along with the couple of small fixes you noted.
03-08-2018
3.1 : OK. 3.2 : I did refer to your comment "replace the instance retrieval in getLogger() with 'new ErrorLogger()'" -- so each getLogger() call will create a new ErrorLogger object and return. Anyway, these different ErrorLoggers will use same System.Logger internally - so on a second thought, I think it is OK to have the implementation where ErrorLogger is kept via a singleton instance. WatchdogTimer.java was removed as part of recent webkit upgrade. I removed it from your patch and applied it to latest codebase - no other change was needed for building.
03-08-2018
Webrev for discussion (non-final): http://cr.openjdk.java.net/~nlisker/8195974/webrev.00/ Except for the following, all changes are from methods that specify a level ( log(Level, ...) ) to the convenience methods ( levelName(...) ). 1. DumpRenderTree changed according to the discussion in the mailing list [1]. 2. WCMediaPlayerImpl + WCMediaPlayer, same as 1. I remind about the possibility of excessive warning loggings. 3. ErrorLoggingUtility + com.sun.javafx.binding.Logging: ErrorLoggingUtility helps with testing error message loggings of some classes in the base module. Instead of having it intercept loggings via a Handler, I expanded on the com.sun.javafx.binding.Logging class to do it. Classes in main call this Logging class to do the logging in the "javafx.beans" logger, so it's a good place to intercept. I created there a subclass of PlatformLogger that overrides the error message loggnings to create a record. (The constructor of PlatformLogger has been made protected so its invocation in subclasses circumvents the map cache, which is not needed anyway, and in the future can be made public (JDK-8200236)). This means that ErrorLoggingUtility is now stateless (I would say a "true" utility class). This brings me to the following: 3.1. Logging is not a test class, but the error logging it does serves only tests. This means that now a needless log record will be kept during normal runtime. The only other approach I see is handling the error loggings in the PlatformLogger itself (as Tom's patch does), but it's still the same issue of doing test stuff during normal runtime. (Since the base module is the only one that needs this functionality for some reason despite other modules having their Logging class, I went with the first approach.) Is this OK? (If so, I'll add comments to the code about this.) 3.2. Inside Logging, ErrorLogger is kept via a singleton instance, which ensures that the content of errorLogRecord is consistent. An alternative is to make the errorLogRecord static and replace the instance retrieval in getLogger() with 'new ErrorLogger()'. This should be fine because, as mentioned in the discussion of JDK-8195799, the System.Logger will take care of fetching the appropriate log (basically, this the same reasoning to get rid of the map caching in PlatformLogger). Is there a preference? 3.3. The main classes that use Logging to log errors only use warning and fine levels. I'm not sure if I should override the others too. Does it make sense to log an error messages into other levels? On the other hand, if someone does do this, they might be confused as to why a record isn't being kept on the other levels. These methods are currently commented out. 3.4. ErrorLoggingUtility has been converted to use only static methods as I didn't see a point in keeping an instance. I didn't change the test classes that use ErrorLoggingUtility, so all the access is via an instance instead of a static access. If the static approach is fine I will change the tests accordingly. 3.5. In the tests, the @BeforeClass { start() } and @AfterClass { stop() } is not needed anymore. I should remove the @AfterClass { stop() } occurrences and change to @BeforeClass { reset() } if everything else is fine. 4. Module-info files changed accordingly. Additional points: * com.sun.javafx.binding.Logging is using "javafx.beans" logger, but javafx.util.converter.LocalDateTimeStringConverter is also using it. * javafx.packager changes were dropped. * There are a lot of licence header changes to be made, is there an automatic way to change the licencing years? (I see now that JDK-8201563 is relevant.) * I believe these changes are consistent with simplification to the follow-up JDK-8200236. [1] http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-May/021784.html
02-08-2018
> 3.1 : Mostly Looks OK. Do you see any performance impact this may cause? I didn't write any performance tests for it, but I can't see how a single instance of ErrorLogger could be a performance issue. In fact, I think that it's the same situation for Controls' com.sun.javafx.scene.control.Logging and Graphics' com.sun.javafx.util.Logging that hold PlatformLoggers used only by tests. Could be that that whole approach is off(?), but out of scope. >3.2 : Inside Logging, ErrorLogger is kept via a singleton instance, which ensures that the content of errorLogRecord is consistent. >There is a chance that this singleton might be accessed by different threads and the errorLogRecord might get corrupted. >I like the other option of having separate loggers using getLoggers() so that each ErrorLogger will have its own errorLogRecord. I don't understand what are these "separate loggers using getLoggers()"? My 2 options would both have a single errorLogRecord. In option 1 it's a non-static field of a singleton (effectively, INSTANCE is a cache), and in option 2 it's a static field of the same log instance retrieved by System.getLogger (this is what guarantees it's the same instance). How would you like to have multiple ErrorLoggers? > There are few minor issues I observed Fixed both. I didn't see them since most replacements in this patch were automatic (with the IDE). On another note, my patch contains com.sun.webkit.WatchdogTimer, but I don't see it anymore. Since this webrev is from ~2.5 months ago, can you help me see if any of the files there were changed so I can target the current revision correctly?
02-08-2018
Hi Nir, I reviewed http://cr.openjdk.java.net/~nlisker/8195974/webrev.00/ Thanks for the exhaustive logger replacement work. Here are my comments : 3.1 : Mostly Looks OK. Do you see any performance impact this may cause? 3.2 : > Inside Logging, ErrorLogger is kept via a singleton instance, which ensures that the content of errorLogRecord is consistent. There is a chance that this singleton might be accessed by different threads and the errorLogRecord might get corrupted. I like the other option of having separate loggers using getLoggers() so that each ErrorLogger will have its own errorLogRecord. 3.3 : Suggest to keep the commented methods 3.4 : OK to modify tests to use static methods of ErrorLoggingUtility. There are few minor issues I observed - 1) In file JavaFXBuilderFactory.java A nit - there should be a space around '+' in changes you have made 2) In file DefaultPlugin.java, The replacement is correct, but the Logger is unused. You can choose to remove the logger altogether from this class.
01-08-2018
I'll review these changes soon. To answer one of your questions: > * There are a lot of licence header changes to be made, is there an automatic way to change the licencing years? (I see now that JDK-8201563 is relevant.) I'll take care of any files that were updated in 2018 in another month or two with JDK-8201563 (and yes, I have scripts to automate this, so I don't have to do it manually).
22-05-2018
Reopening this bug (I edited the wrong bug by mistake).
22-05-2018
> Your patch shows an ad-hoc replacement for the case in ErrorLoggingUtiltity (though I believe the result is not the same as current code, granted it's a PoC). For me the JUnit-Tests pass with the change so the behavior is the same, we need to remember that we only talk about JUnit-Tests so it is a very controlled environment.
05-05-2018
> Dumb question but couldn't we move this backtracking code PlatformLogger If you mean to ask if we can replace the current usages of util.logging classes by expanding PlatformLogger then that's the whole issue here. Your patch shows an ad-hoc replacement for the case in ErrorLoggingUtiltity (though I believe the result is not the same as current code, granted it's a PoC). The way I'm trying to approach it is first to see what the maintainer thinks about the current code. Sometimes there are relics that can be cleaned up or removed. In the case of WCMediaPlayer it seems possible to just remove and simplify the extra logging utilities, and it might be possible to do so for other cases. If we just replace everything we will carry relics forward, which I would like to avoid (at the cost of time-to-fix). Once we're left with the must-stay code, we can either provide our own replacements for j.u.l (as you did), or shove all of the j.u.l dependencies into the PlatformLogger. The former has the advantage of getting rid of j.u.l compile-time dependencies, but has the disadvantages of reinventing the wheel and the extra maintenance that come with providing your own implementation. If JavaFX really needs the behavior that the classes in j.u.l provide then writing our own replacements doesn't seem right (and then you can use the same argument about other dependencies - why use them when you can copy-paste the code into your own classes?). On the other hand, if JavaFX only needs some small bookkeeping and j.u.l was just a convenience, we can use replacements. I myself am not completely clear on where it stands yet. For example, DumpRenderTree writes to a file, uses a formatter, and checks all writes with isDebug(). Is all this ceremony really needed? Do we have to write to a file and do we have to use a formatter? Who uses this file? I don't know, which is why I asked for evaluation (in this case from Murali). Maybe we don't need to move code to PlatformLogger in the first place.
05-05-2018
PoC to add back-tracking to PlatformLogger
03-05-2018
Dumb question but couldn't we move this backtracking code PlatformLogger
03-05-2018
As much as I agree that a project should not change its code just because an IDE can not deal with it the much i appreciate if I could simply contribute to OpenJFX through my (and my others!) preferred IDE. I for myself was not able to make Eclipse happy unless I added the import-static to my module-info.java. One could argue that JavaFX JUnit-Test rely on the fact that System.Logger logs with JUL which is something that might never change (although in theory it could :-)
03-05-2018
> As long as it is only used by test classes, and not code in any javafx.* module, this can be left alone. Tom, can you weight in on this? I think even code in tests will cause problems with Eclipse. If we use --add-exports/reads in the .classpath, will that be enough? (Though there is a limit on how much we can force code changes only because the IDE chose to include test code in the module.) > are there any other places where Handlers, etc., are used? Only in DumpRenderTree and HelloService.
03-05-2018
> test.com.sun.javafx.binding.ErrorLoggingUtiltity. This type of log is used by test classes for properties and bindings to test for exceptions, as much as I see. As long as it is only used by test classes, and not code in any javafx.* module, this can be left alone. The primary goal is to make it so that none of the javafx.* modules need to "requires java.util.logging". Since the tests run in the unnamed module, there shouldn't be any concern. If it's easy to fix, then that's OK, but I wouldn't spend too much effort on it. > com.sun.javafx.webkit.drt.DumpRenderTree. Inside `main`, seems to be running some test. Yes, this is one that will need more investigation. Ideally, all of the setup code for logging would be removed, but Murali will need to comment on this. > com.sun.media.jfxmedia.logging.Logger. There's a mapping from native logging levels to util.logging.Levels. Despite the comments, there are no references to any logging framework here that I can see. The methods and fields are all internal to the javafx.media module. > In the packager in hello.HelloService. There's logging into a file. This is a test application, so you can leave it alone. > Since the System.Logger or the PlatformLogger don't have the facilities to replace Handlers, Managers, Records etc. we can either write ad-hoc replacements, or change the behavior. It could be that using only the System.Logger as in JDK-8200236 is not an option. I'd prefer the former (ad-hoc replacements) unless there is a compelling need to keep the current functionality. Other than DumpRenderTree, which is only used for testing, and is excluded from the runtime, are there any other places where Handlers, etc., are used?
03-05-2018
The other affected places where I don't know what the required behavior is or if it's old code that should be removed are: * test.com.sun.javafx.binding.ErrorLoggingUtiltity. This type of log is used by test classes for properties and bindings to test for exceptions, as much as I see. * com.sun.javafx.webkit.drt.DumpRenderTree. Inside `main`, seems to be running some test. * com.sun.media.jfxmedia.logging.Logger. There's a mapping from native logging levels to util.logging.Levels. * In the packager in hello.HelloService. There's logging into a file. Since the System.Logger or the PlatformLogger don't have the facilities to replace Handlers, Managers, Records etc. we can either write ad-hoc replacements, or change the behavior. It could be that using only the System.Logger as in JDK-8200236 is not an option.
03-05-2018
Following the discussion on the mailing list [1], Iv'e removed the j.u.l dependency from WCMediaPlayer and WCMediaPlayerImpl by removing the static initialization block along with the `verbose` flag, and changing INFO level loggings to FINE level. As noted above, JDK-8198470 should be fixed with this change. [1] http://mail.openjdk.java.net/pipermail/openjfx-dev/2018-May/021784.html
03-05-2018
Thanks Kevin, did not know about JDK-8198470. Removing setLevel is not a problem, but what behavior do you want me to code for? * Reading a boolean from the log config file that sets `verbose` (on/off) and removing the minimum level parts. * Reading a Level from the log config file and if it's OFF or null then set `verbose` to false, and true otherwise. * Remove the block including `verbose` and log everything? * Something else?
21-04-2018
I note that this static block is currently causing a bug when running with a security manager. See JDK-8198470. It might be OK to simply remove this static block. If it is kept, then the code that sets the level explicitly should be removed (library code should not set log levels). Once the setting of the log levels is removed, then JDK-8198470 should be retested, and if the problems goes away as expected, JDK-8198470 can be closed as a duplicate.
21-04-2018
Related to 4, com.sun.webkit.graphics.WCMediaPlayer has a static initialization block static { log = PlatformLogger.getLogger("webkit.mediaplayer"); if (log.getLevel() == null) { // disable logging if webkit.mediaplayer.level // is not specified (explicitly) in log config file verbose = false; log.setLevel(Level.OFF); } else { verbose = true; log.log(Level.CONFIG, "webkit.mediaplayer logging is ON, level: {0}", log.getLevel()); } } I didn't build Webkit and I don't know what the "log config file" is, but I assume a util.Logger could be created sometime before this code is executed. If that's the case, the underlying mechanism needs to be changed to use System.Logger. The problem is that System.Logger is stateless while util.Logger holds a util.Level (System.Logger has no getLevel()). To solve this, the config file could specify a System.Logger.Level independently and this level will be checked along with `verbose` on each logging attempt. Note that since the Level is used to specify the minimal level for logging, we can't use the new PlatformLogger because its Levels do not implement Comparable, while System.Logger.Levels do, and we need it to mimic the internal behavior of the util.Logger's minimal level. Maybe this is fine because JDK-8200236 suggest to use the System.Logger directly anyway. Or, we can change the PlatformLogger's Levels. All that is if the current behavior is meant to be kept. `verbose` seems redundant - it is used as a check if logging should happen before each attempt, but if the Level is OFF nothing will be logged anyway.
21-04-2018
[~nlisker] Yes, please assign it to yourself. About the fix: 1. The jdk.packager and javafx.base modules also use java.util.logging, can they be changed to? [Ajit] Yes for javafx.base. @Kevin, can you please confirm about jdk.packager? 2. The above 2 use classes such as java.util.logging.Handler and java.util.logging.LogRecord that don't have a parallel in platform/system logger, what to do with these? [Ajit] We should try to find a way to get rid of them if possible - I think, it is not straight forward and will need some workaround. 3. module-info.java of javafx.base needs to add the treated modules to "exports com.sun.javafx.logging to", is this fine? [Ajit] Yes, it is. 4. The platform logger doesn't have a Level.CONFIG which is present in util.logging, should it be added or replaced by an existing level? [Ajit] Level.CONFIG is equivalent to System.Logger.Level.DEBUG which is mapped to com.sun.javafx.logging.PlatformLogger.Level.FINE So you can use- PlatformLogger.fine() method in this case. 5. Similar to 4, how to replace log.log(Level.ALL, a, b) when there is no way to log ALL. [Ajit] I see only a single occurrence of this in file - modules\javafx.web\src\main\java\com\sun\javafx\webkit\theme\RenderThemeImpl.java Why do we need to log a particular message at level ALL? can we get rid of this? First option would be to replace this call with any specific log methods in PlatformLogger. Can we settle for some level other than ALL for this? If we cannot replace this log call, then another approach would be to add a method to com.sun.javafx.logging.PlatformLogger that passes the call to underlying loggerProxy using Level.ALL.
20-04-2018
Making the needed changes to the jdk.packager are fine, but not strictly necessary. If it's easy, then go ahead and make the changes. I definitely agree with the idea of not using log level ALL. In looking at the usage, SEVERE would seem a good replacement.
20-04-2018
Ajit, I'm starting to work on a fix, can I take the Assignee? About the fix: 1. The jdk.packager and javafx.base modules also use java.util.logging, can they be changed to? 2. The above 2 use classes such as java.util.logging.Handler and java.util.logging.LogRecord that don't have a parallel in platform/system logger, what to do with these? 3. module-info.java of javafx.base needs to add the treated modules to "exports com.sun.javafx.logging to", is this fine? 4. The platform logger doesn't have a Level.CONFIG which is present in util.logging, should it be added or replaced by an existing level? 5. Similar to 4, how to replace log.log(Level.ALL, a, b) when there is no way to log ALL.
19-04-2018
Not yet - to make things work temporarily I added "requires static" and then I was able to run the test suite
18-04-2018
Tom, I wonder if we're hitting the same issue. For me, in the controls module, the compilation unit test.javafx.scene.control.ToggleButtonTest gives an error "The type java.util.Locale cannot be resolved. It is indirectly referenced from required .class files" because of the imports from java.util.logging. Did you work on a fix? If not, I believe I can do it quickly.
18-04-2018
[~tschindl] You are most welcome to contribute to this RFE.
28-03-2018
I'd volunteer to provide fixes for that. The reason I'm interested in that is that it makes my IDE setup easier because this optional compile dependency on another module makes eclipse freak out a bit ;-)
27-03-2018

Blocks :	JDK-8195799 - Use System logger instead of platform logger in javafx modules
Duplicate :	JDK-8198470 - AccessControlException when running SandboxAppTest
Relates :	JDK-8200236 - Simplify/Replace com.sun.javafx.logging.PlatformLogger class & its usage
Relates :	JDK-8198470 - AccessControlException when running SandboxAppTest
Relates :	JDK-8209036 - systemTests should use PlatformLogger instead of java.util.logging