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

[Xuelei Fan]

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
>