SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

[Xuelei Fan]

Update: http://hg.openjdk.java.net/jdk/sandbox/rev/e4fe7c97b1de

On 6/9/2018 2:42 AM, Seán Coffey wrote:
> Some comments on SSLLogger also :
> 
> formatCaller() uses getStackTrace() to walk the stack. It's probably 
> more expensive than using the newer Stackwalker class. Could it be 
> replaced with something like :
> 
>>          return StackWalker.getInstance().walk(s ->
>>                  s.dropWhile((f -> 
>> f.getClassName().startsWith("sun.security.ssl.SSLLogger")))
>>                          .map(f -> f.getClassName() + ":" + 
>> f.getLineNumber())
>>                          .findFirst().orElse("unknown caller"));
> 
Good point!

> Looks like this method is not used :
> 
>>   406         private static boolean isLoggerImpl(StackTraceElement ste) {
>>   407             String cn = ste.getClassName();
>>   408             System.out.println("class name: " + cn);
>>   409             return cn.equals("sun.security.ssl.SSLLogger");
>>   410         }
> 
Removed.

> Also the SSLSimpleFormatter class has dateFormat variable stored in a 
> ThreadLocal. Is there a reason for that ?
> 
SimpleDateFormat is not multiple thread-safe.  There were some expected 
issues if using SimpleDateFormat instance directly in multiple threads 
environment.

Thanks,
Xuelei

> regards,
> Sean.
> 
> 
> On 08/06/2018 02:40, Xuelei Fan wrote:
>> I got your ideas.  Let's see if we can make a change, a little bit later.
>>
>> Thanks,
>> Xuelei
>>
>> On 6/7/2018 6:23 PM, Weijun Wang wrote:
>>> My final comment on this class:
>>>
>>> Since SSLConsoleLogger is a Logger child, its log(..., params) method 
>>> should follow the Logger::log spec, which has
>>>
>>>          /**
>>>           * Logs a message with resource bundle and an optional list of
>>>           * parameters.
>>>           * ...
>>>           * @param format the string message format in {@link
>>>           * java.text.MessageFormat} format, (or a key in the message
>>>           * catalog if {@code bundle} is not {@code null}); can be 
>>> {@code null}.
>>>           * @param params an optional list of parameters to the 
>>> message (may be
>>>           * none).
>>>           * ...
>>>           */
>>>          public void log(Level level, ResourceBundle bundle, String 
>>> format,
>>>                  Object... params);
>>>
>>> Especially, the String parameter is in MessageFormat format (that's 
>>> why it's named "format" but not "message"). However, 
>>> SSLConsoleLogger::log is always called with format being a plain 
>>> string and params are stacked in the output separated by command and 
>>> newline.
>>>
>>> My opinion is that this is not the normal style of using a Logger and 
>>> we should not allow system logger (through -Djavax.net.debug=). If 
>>> you still see benefit in that, can we make this change at the moment?
>>>
>>> -                    logger.log(level, msg, formatted);
>>> +                    if (logger instanceof SSLConsoleLogger) {
>>> +                        logger.log(level, msg, formatted);
>>> +                    } else {
>>> +                        logger.log(level, msg + ": " + formatted);
>>> +                    }
>>>
>>> Thanks
>>> Max
>>>
>>>> On Jun 7, 2018, at 11:24 AM, Weijun Wang <weijun.wang at oracle.com> 
>>>> wrote:
>>>>
>>>> I think I understand. So if a user sets it to empty, they will also 
>>>> need to create a customized logger/formatter like SSLLogger that is 
>>>> able to output a parameter without a placeholder in msg.
>>>>
>>>> --Max
>>>>
>>>>> On Jun 7, 2018, at 9:51 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>>>>
>>>>> In this implementation, the SunJSSE logging is updated to (the 
>>>>> class comment lines):
>>>>>
>>>>> /**
>>>>> * Implementation of SSL logger.
>>>>> *
>>>>> * If the system property "javax.net.debug" is not defined, the 
>>>>> debug logging
>>>>> * is turned off.  If the system property "javax.net.debug" is 
>>>>> defined as
>>>>> * empty, the debug logger is specified by 
>>>>> System.getLogger("javax.net.ssl"),
>>>>> * and applications can customize and configure the logger or use 
>>>>> external
>>>>> * logging mechanisms.  If the system property "javax.net.debug" is 
>>>>> defined
>>>>> * and non-empty, a private debug logger implemented in this class 
>>>>> is used.
>>>>> */
>>>>>
>>>>> On 6/6/2018 6:37 PM, Weijun Wang wrote:
>>>>>> But will any application use the logger named "javax.net.debug"?  
>>>>>> I think that's only for JSSE.
>>>>> If using System Logger, the "javax.net.debug" system property will 
>>>>> be used to turn on/off the logger (define with empty or not defined).
>>>>>
>>>>> Xuelei
>>>>>
>>>>>>> On Jun 7, 2018, at 9:35 AM, Xuelei Fan<xuelei.fan at oracle.com>  
>>>>>>> wrote:
>>>>>>>
>>>>>>> I see your concerns now.  Currently, we don't fine the format if 
>>>>>>> using System logger.  Applications can define the format and 
>>>>>>> output style for themselves if needed. That's also the purpose 
>>>>>>> why we introduce the System Logger in the provider.
>>>>
>>>
>