On Sat, 8 May 2021 00:21:54 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Alexey Bakhtin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add Cache.pull method
>
> src/java.base/share/classes/sun/security/ssl/PreSharedKeyExtension.java line
> 377:
>
>> 375: // If we are keeping state, see if the identity is
>> in the cache
>> 376: if (requestedId.identity.length ==
>> SessionId.MAX_LENGTH) {
>> 377: s = sessionCache.pull(requestedId.identity);
>
> Would you please add a comment here so as we know why a pull method could be
> used here? For example:
>
>
> - // If we are keeping state, see if the identity is in
> the cache
> + // If we are keeping state, see if the identity is in the
> + // cache. Note that for TLS 1.3, we would also clean
> + // up the cached session if it is not rejoinable.
Thank you, added comments as suggested.
> src/java.base/share/classes/sun/security/ssl/SSLSessionContextImpl.java line
> 183:
>
>> 181: if (id != null)
>> 182: return sessionCache.pull(new SessionId(id));
>> 183: return null;
>
> As this is an internal method, it should be safe to assume that the id is
> non-null. I'm fine if you want to keep the non-null check, but please use
> braces for if-statement (see also
> https://www.oracle.com/java/technologies/javase/codeconventions-statements.html#449).
Thank you. Added braces but I'd like to keep non-null check.
> src/java.base/share/classes/sun/security/util/Cache.java line 430:
>
>> 428: return null;
>> 429: }
>> 430: V value;
>
> I may add a blank line before this line.
Thank you, added
> src/java.base/share/classes/sun/security/util/Cache.java line 442:
>
>> 440: entry.invalidate();
>> 441: return value;
>> 442: }
>
> I may adjust the lines a little bit so as to avoid duplicated operations (see
> the implementation code of isValid()).
>
>
> long time = (lifetime == 0) ? 0 : System.currentTimeMillis();
> if (entry.isValid(time)) {
> V value = entry.getValue();
> entry.invalidate();
> return value;
> } else {
> if (DEBUG) {
> System.out.println("Ignoring expired entry");
> }
> return null;
> }
I'd like to keep my code as-is. We still need invalidate() if entry is not
valid (see remove() operation).
-------------
PR: https://git.openjdk.java.net/jdk/pull/3664
