RFR: 8303682: Simplify HttpClient DebugLogger [v2]
Daniel Fuchs
dfuchs at openjdk.org
Wed Mar 8 11:29:44 UTC 2023
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
More information about the net-dev
mailing list