RFR: 8044609: javax.net.debug options not working and documented as expected [v26]

[Sean Coffey]

On Tue, 27 Jan 2026 23:28:39 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> Sean Coffey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 48 commits:
>> 
>>  - correct NamedGroup.java merge
>>  - Merge branch 'master' into 8044609-ssl
>>  - fix up test bug ID
>>  - fix up files post merge
>>  - Merge branch 'master' into 8044609-ssl
>>  - prep for isOn() merge
>>  - Merge branch 'master' into 8044609-ssl
>>  - Merge branch 'master' into 8044609-ssl
>>  - Merge branch 'master' into 8044609-ssl
>>  - Incorporate review comments from Brad
>>  - ... and 38 more: https://git.openjdk.org/jdk/compare/c69275dd...6b5c692c
>
> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 62:
> 
>> 60: public final class SSLLogger implements System.Logger {
>> 61:     private static final System.Logger logger;
>> 62:     // high level boolean to track whether "all" or "ssl" option
> 
> Minor nit:  
> This makes it sounds like the boolean state `true` might be `all` and `false` is `ssl`.
> 
> Suggest maybe "to track whether logging is active (i.e. all/ssl)."

done

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 105:
> 
>> 103:                     if (Opt.HANDSHAKE.on && p.contains("verbose")) {
>> 104:                         Opt.HANDSHAKE_VERBOSE.on = true;
>> 105:                     }
> 
> Not absolutely needed given the options currently defined, but you could `replaceFirst()` on these 3 as well.  
> 
> Your call.

thanks - I might leave this out for now. Unlikely we'll ever need such values to be removed.

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 130:
> 
>> 128:                 }
>> 129:             }
>> 130:             // javax.net.debug would be misconfigured property with respect
> 
> Nit:  add extra line.

done

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 149:
> 
>> 147:         return logging;
>> 148:     }
>> 149:     /**
> 
> Nit:  add a line.

done

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 237:
> 
>> 235:         System.err.printf("%nAdding valid filter options to \"ssl\" will log" +
>> 236:                 " messages to include%njust those filtered categories.%n");
>> 237:         System.err.printf("%nIf \"ssl\" is specified by itself," +
> 
> Maybe reverse 235 and 237.  ssl by itself, then ssl+filter?  I've gone back/forth on this quite a bit.  Do you have a preference?

good point. I've swapped these around. I think it reads well now.

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 296:
> 
>> 294:         return level != Level.OFF;
>> 295:     }
>> 296:     /**
> 
> Nit:  add a line.

done

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 311:
> 
>> 309:      * Enabling subcomponents fine-tunes (filters) debug output.
>> 310:      */
>> 311:     public enum Opt {
> 
> Does this need to be public?

Looks like there's some usage of SSLLogger outside of sun.security.ssl

e.g. sun/security/util/DomainName.java

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 465:
> 
>> 463:                         formatParameters(parameters) :
>> 464:                         Utilities.indent(formatParameters(parameters)))
>> 465:                 };
> 
> Why did the indent get changed?

not sure! I've reverted the indentation back

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 490:
> 
>> 488:                     isFirst = false;
>> 489:                 } else {
>> 490:                     builder.append(",\n");
> 
> Why did this get changed from `LINE_SEP` to `\n`.  All of the other line separators in this file are still `LINE_SEP`.

this might have crept back in during a merge operation. Resolved.

> src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 494:
> 
>> 492: 
>> 493:                 if (parameter instanceof Throwable) {
>> 494:                     builder.append(formatThrowable((Throwable)parameter));
> 
> Nit:  from the Java Coding conventions, Section 8.2:
> 
>     Casts should be followed by a blank. Examples:
>         myMethod((byte) aNum, (Object) x);
>         myFunc((int) (cp + 5), ((int) (i + 3))
> 
> I don't have a strong preference here.  
> 
> If you do revert back, there are several places that got changed here.

Thanks - I need to remember that coding convention. Seen different styles.
I've taken an extra step here and introduced instanceof pattern matching where possible. Cleans up the code some more.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737388191
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737392636
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737392983
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737393402
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737395056
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737395731
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737398470
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737400506
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737402184
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2737405959