On Thu, 23 Feb 2023 15:08:55 GMT, Daniel Jeliński <[email protected]> wrote:
>> src/java.base/share/classes/sun/net/www/http/KeepAliveCache.java line 321:
>>
>>> 319: /* return a still valid, idle HttpClient */
>>> 320: HttpClient get() {
>>> 321: // check the most recent connection, use if still valid
>>
>> It might be prudent to assert that the global cacheLock is held by the
>> current thread in get & put (if that's feasible without too much hackery).
>> Just worrying about future maintainers possibly modifying this code and
>> using ClientVector outside of the lock. The assert would make sure they
>> ponder the consequences.
>
> Asserts added; let me know if the amount of hackery involved looks acceptable.
Looks reasonable. I personally dislike having several top-level classes in the
same file so I'd say having ClientVector as an inner class is a plus. Having
ClientVector carry a (hidden) pointer to its parent instance shouldn't be an
issue since ClientVector shouldn't escape. I'd say this is good, provided that
the modified test still passes. Otherwise, we can replace the assert with a
simple comment, to warn that the class is not thread-safe and should not be
used outside of blocks protected by the cacheLock.
-------------
PR: https://git.openjdk.org/jdk/pull/12676
