On Fri, 31 Mar 2023 14:47:08 GMT, Daniel Jeliński <[email protected]> wrote:
>> Please review this fix for a regression in HttpExchange's setAttribute >> method. >> >> The specification of setAttribute explicitly states that null values are >> allowed. >> JDK-8266897 changed `attributes` to `ConcurrentHashMap`, which does not >> allow null values. >> >> This patch modifies `setAttribute` to remove the attribute from the map when >> null value is requested. >> >> A new test was added to verify that setting attributes works as expected >> both for null and non-null values. >> >> Tier1-3 clean. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Use URIBuilder The changes look good to me. I have added a trivial comment about the test inline. test/jdk/com/sun/net/httpserver/ExchangeAttributeTest.java line 29: > 27: * @summary Tests for HttpExchange set/getAttribute > 28: * @library /test/lib > 29: * @run junit/othervm ExchangeAttributeTest Hello Daniel, I didn't see anything specific in the test which would need `othervm`. Is this intentional? ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13264#pullrequestreview-1368292029 PR Review Comment: https://git.openjdk.org/jdk/pull/13264#discussion_r1155484397
