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

[Weijun Wang]

> 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?

If so, I'm OK with the current structure. 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