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

[Weijun Wang]

Have you updated it yourself? I have a proposal at

   http://cr.openjdk.java.net/~weijun/9999999/webrev.hello-cookie-manager/

--Max

> On Jun 6, 2018, at 10:40 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> On 6/5/2018 8:48 PM, Weijun Wang wrote:
>>> On 6/5/2018 6:54 AM, Weijun Wang wrote:
>>>> HelloCookieManager.java:
>>>>   44     HelloCookieManager(SecureRandom secureRandom) {
>>>>   45         this.secureRandom = secureRandom;
>>>>   46     }
>>>>   47
>>>>   48     HelloCookieManager valueOf(ProtocolVersion protocolVersion) {
>>>>   Why not just create a static method and make HelloCookieManager abstract?
>>>>    static HelloCookieManager of(SecureRandom secureRandom, ProtocolVersion protocolVersion)
>>>> I haven't seen "raw" HelloCookieManager used anywhere.
>>> For every SSLContext, the SecureRandom may be different, so we need a different HelloCookieManager instance and cookie managers for each SSLContext instance:
>>> 
>>> SSLContextImpl.java
>>> -------------------
>>>  253   helloCookieManager = new HelloCookieManager(secureRandom);
>> So the main benefit of the current implementation is that once a HelloCookieManager is created you can reuse the 3 child HelloCookieManager objects inside it. Right?
> Yes.
> 
>> If so, I'm OK with the current structure.
> I see you concerns.  The structure looks weird in some sense, although it is easier for a package private class.  Let me see if I can make an update in the next changeset.
> 
> Thanks,
> Xuelei
> 
>> My major concern is, since the "base" HelloCookieManager is never useful we should take great care not to create an instance of it. How about this?
>> 1. Make HelloCookieManager abstract.
>> 2. Create an non-abstract inner class HelloCookieManager$Builder and move the 3 real instances there.
>> 3. Inside SSLContextImpl, store a field of HelloCookieManager$Builder, and in getHelloCookieManager(), call "builder.get(protocolVersion)" to return one of the 3 real instances.
>> If the reuse of the 3 real instances is not a big benefit, you can just make HelloCookieManager abstract and create a new child instance in every getHelloCookieManager() call.
>> Anyway, I feel uncomfortable that the base HelloCookieManager class can be instantiated.
>> Thanks
>> Max
>>>>> On Jun 5, 2018, at 12:12 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>>>> 
>>>>>> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01