RFR: 8303682: Simplify HttpClient DebugLogger [v2]

[Daniel Fuchs]

On Wed, 8 Mar 2023 07:26:22 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Daniel Fuchs has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Make sure julLogger is not GC'ed before the end of the test.
>>  - Integrated review feedback
>
> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 70:
> 
>> 68:         // true if at least on of the three levels is not Level.OFF
>> 69:         public boolean on() {
>> 70:             return minSeverity() <= Level.OFF.getSeverity();
> 
> I suspect this should instead be `return minSeverity() < Level.OFF.getSeverity();` i.e. if min severity is `OFF` then on() shouldn't return `true`.

Doh! Done

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 98:
> 
>> 96:         public LoggerConfig withLogLevel(Level logLevel) {
>> 97:             return new LoggerConfig(outLevel, errLevel, logLevel);
>> 98:         }
> 
> To avoid shadowing, do you think we could rename the method params of these 3 methods to `newErrLevel`, `newOutLevel` and `newLogLevel`? If you prefer the current form, that's fine too.

I like shadowing here ;-)

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 103:
> 
>> 101:         public static final LoggerConfig STDERR = new LoggerConfig(Level.OFF, Level.ALL, Level.OFF);
>> 102:         /** Logs on {@link System#out} only, does not forward to System.Logger **/
>> 103:         public static final LoggerConfig STDOUT = new LoggerConfig(Level.OFF, Level.ALL, Level.OFF);
> 
> This looks like an oversight. I think this should have been:
> 
> new LoggerConfig(Level.ALL, Level.OFF, Level.OFF);

argh. good catch!

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 122:
> 
>> 120:             new DebugLogger(HTTP, "WS"::toString, LoggerConfig.OFF);
>> 121:     private static final DebugLogger NO_HPACK_LOGGER =
>> 122:             new DebugLogger(HTTP, "HPACK"::toString, LoggerConfig.OFF);
> 
> Not because of this PR, but I think the NO_WS_LOGGER and the NO_HPACK_LOGGER have a typo/bug in the `System.Logger`, they are using. I think the NO_WS_LOGGER should be:
> 
> new DebugLogger(WS, "WS"::toString, LoggerConfig.OFF);
> 
> and the NO_HPACK_LOGGER should be:
> 
> 
> new DebugLogger(HPACK, "HPACK"::toString, LoggerConfig.OFF);

Good catch. Doesn't really matter since these loggers do nothing though.

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 161:
> 
>> 159:      *
>> 160:      * @param logger The internal logger to which messages will be forwarded.
>> 161:      *               This should be either {@link #WS}, {@link #HPACK}, or {@link #HTTP};
> 
> Given that the constructor of `DebugLogger` is private and we only expose APIs to create `HTTP`, `WS` and `HPACK` loggers, this statement that the loggers be either of these is true. However, the constructor isn't doesn't do any checks for the `logger` instance (other than null check). Do you think we can drop this sentence?

Reworded: `This is typically either ... `

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 165:
> 
>> 163:      * @param dbgTag A lambda that returns a string that identifies the caller
>> 164:      *               (e.g: "SocketTube(3)", or "Http2Connection(SocketTube(3))")
>> 165:      * @param config The levels above which messages will be printed to the
> 
> This should be `The levels at or above which ....`

Done.

> src/java.net.http/share/classes/jdk/internal/net/http/common/DebugLogger.java line 198:
> 
>> 196:     }
>> 197: 
>> 198:     static boolean levelEnabledFor(Level level, LoggerConfig config,
> 
> It looks like this can be made `private`

Done

> src/java.net.http/share/classes/jdk/internal/net/http/common/Utils.java line 975:
> 
>> 973:      */
>> 974:     public static Logger getWebSocketLogger(Supplier<String> dbgTag) {
>> 975:         return DebugLogger.createWebSocketLogger(dbgTag, DEBUG_CONFIG);
> 
> I think this should should be:
> 
> 
> return DebugLogger.createWebSocketLogger(dbgTag, DEBUG_WS_CONFIG);
> 
> i.e. `DEBUG_WS_CONFIG` instead of `DEBUG_CONFIG`

Good catch!

> test/jdk/java/net/httpclient/DebugLoggerTest.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved.
> 
> Should just be `2023`

Done

> test/jdk/java/net/httpclient/DebugLoggerTest.java line 85:
> 
>> 83:      * @apiNote
>> 84:      * For instance, a {@code RecordingPrintStream} might be used as an
>> 85:      * interceptor to record anything printed on {@code }System.err}
> 
> Typo, should have been `{@code System.err}` instead of `{@code } System.err}`

Good catch

> test/jdk/java/net/httpclient/DebugLoggerTest.java line 247:
> 
>> 245:         julLogger.setLevel(Level.ALL);
>> 246:         julLogger.setUseParentHandlers(false);
>> 247:         julLogger.addHandler(logHandler);
> 
> Given our previous experience where a `j.u.l.Logger` getting garbage collected would result in odd failures in tests, do you think we should keep reference to this `julLogger` till the end of the test? Right now, I don't see this reference being used after this line.

Good point. Will fix.

> test/jdk/java/net/httpclient/DebugLoggerTest.java line 289:
> 
>> 287:         if (dest.contains(Destination.ERR)) {
>> 288:             if (!errStr.contains(msg)) {
>> 289:                 throw new AssertionError("stderr does not contain the expected message");
> 
> Do you think we should print the errStr/outStr/logs in these assertion failure messages, to help debug any failures?

I believe the stack trace should be enough: the line number should reveal which message was not found.

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

PR: https://git.openjdk.org/jdk/pull/12900