8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear #22454

jaikiran · 2024-11-29T09:13:31Z

Can I please get a review of this doc-only change which proposes to clarify the behaviour of com.sun.net.httpserver.HttpExchange.setAttribute() and com.sun.net.httpserver.HttpExchange.getAttribute() methods?

As noted in https://bugs.openjdk.org/browse/JDK-8235786, it's not clear from the javadoc of these methods that the attributes that are set or retrieved through the exchange are actually those that belong to the entire com.sun.net.httpserver.HttpContext and thus are shared by all exchanges.

The commit in this PR specifies this behaviour to match the current implementation. I'll file a CSR once we settle on the text.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8345243 to be approved

Issues

JDK-8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear (Bug - P4)
JDK-8345243: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear (CSR)

Reviewers

Daniel Fuchs (@dfuch - Reviewer)
Michael McMahon (@Michael-Mc-Mahon - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22454/head:pull/22454
$ git checkout pull/22454

Update a local copy of the PR:
$ git checkout pull/22454
$ git pull https://git.openjdk.org/jdk.git pull/22454/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22454

View PR using the GUI difftool:
$ git pr show -t 22454

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22454.diff

Using Webrev

Link to Webrev Comment

…ibute is unclear

jaikiran · 2024-11-29T09:13:40Z

/csr needed

bridgekeeper · 2024-11-29T09:14:04Z

👋 Welcome back jpai! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-29T09:14:17Z

@jaikiran This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear

Reviewed-by: dfuchs, michaelm

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 33 new commits pushed to the master branch:

7c944ee: 8345172: x86: Some CPU feature asserts are declared as 32-bit only
67f18cc: 8345142: Remove uses of SecurityManager in Printing related classes
352201d: 8343788: Provide means to alter lib/tzmappings entries on Windows
29c57e8: 8342677: Add IR validation tests for newly added saturated vector add / sub operations
30b8bbe: 8345060: Remove Security Manager dependencies from java.security.KeyStore and Identity APIs and implementations
1ca7644: 8339480: Build static-jdk image with a statically linked launcher
d589baf: 8345218: Clean out references to windows-x86 in jib profiles
b823398: 8345267: Fix memory leak in JVMCIEnv dtor
9a48e4d: 8345177: RISC-V: Add gtests for cmpxchg
e3b679a: 8345176: Add tests to verify java.net.Socket constructors close the socket on failure
... and 23 more: https://git.openjdk.org/jdk/compare/959fa4a1a35a1bb650ec5888efaf3d0fc8cfb025...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-29T09:14:49Z

@jaikiran has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@jaikiran please create a CSR request for issue JDK-8235786 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

openjdk · 2024-11-29T09:15:25Z

@jaikiran The following label will be automatically applied to this pull request:

net

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-29T09:18:09Z

Webrevs

dfuch

Thanks for this clarification. I was bitten by that too the first time I used these attributes.

jaikiran · 2024-11-29T10:50:15Z

The CSR is now ready for review https://bugs.openjdk.org/browse/JDK-8345243

Michael-Mc-Mahon · 2024-11-29T12:24:57Z

src/jdk.httpserver/share/classes/com/sun/net/httpserver/HttpExchange.java

@@ -233,22 +233,28 @@ protected HttpExchange() {
    public abstract String getProtocol();

    /**
-     * {@link Filter} modules may store arbitrary objects with {@code HttpExchange}
-     * instances as an out-of-band communication mechanism. Other filters
+     * {@return the attribute's value from this exchange's


It looks a bit odd in the generated doc that this statement appears as the summary and is then repeated verbatim in the @returns. Could the first sentence be a shorter summary?

Also, since we are clarifying the spec here, could a small test be added?

Done - I've updated the @return to split it into separate sentences and also updated an existing test to include the testing for HttpExchange.getAttribute() and setAttribute(). The test continues to pass. I'll run it through our CI too.

dfuch

Still looks good (and even better) :-)

…ibute()

Michael-Mc-Mahon

Looks good!

jaikiran · 2024-12-03T02:35:40Z

Thank you Michael and Daniel for the reviews. The CSR has been approved and tier1 and tier2 testing has passed with this change. I'll go ahead and integrate this now.

jaikiran · 2024-12-03T02:35:48Z

/integrate

openjdk · 2024-12-03T02:36:17Z

Going to push as commit 40ae469.
Since your change was applied there have been 45 commits pushed to the master branch:

24983dd: 7038838: Unspecified NPE in java.net.IDN methods
325366e: 8345141: Remove uses of SecurityManager in ShellFolder related classes
d88c7b3: 8345279: Mistake in javadoc of javax.sql.rowset.BaseRowSet#setBigDecimal
3f6c042: 8345143: Remove uses of SecurityManager in the java.desktop module
5958463: 8343377: Performance regression in reflective invocation of native methods
68b1b94: 8344904: Interned strings in old classes are not stored in CDS archive
1997e89: 8345346: Shenandoah: Description of ShenandoahGCMode still refers to incremental update mode
3a3bcd5: 8344800: Add W3C DTDs and XSDs to the JDK built-in Catalog
940aa7c: 8344397: Remove Security Manager dependencies from java.security and sun.security packages
3d0d0e6: 8345012: os::build_agent_function_name potentially wastes a byte when allocating the buffer
... and 35 more: https://git.openjdk.org/jdk/compare/959fa4a1a35a1bb650ec5888efaf3d0fc8cfb025...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-03T02:36:25Z

@jaikiran Pushed as commit 40ae469.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttr…

Loading
Loading status checks…

e5837da

…ibute is unclear

openjdk bot added rfr csr labels Nov 29, 2024

openjdk bot added the net label Nov 29, 2024

dfuch approved these changes Nov 29, 2024

View reviewed changes

Michael-Mc-Mahon reviewed Nov 29, 2024

View reviewed changes

Michael's suggestion - shorten the "@return"

Loading
Loading status checks…

6628c51

dfuch approved these changes Nov 29, 2024

View reviewed changes

update an existing test to verify HttpExchange.getAttribute()/setAttr…

Loading
Loading status checks…

3123c33

…ibute()

dfuch approved these changes Nov 29, 2024

View reviewed changes

Michael-Mc-Mahon approved these changes Nov 29, 2024

View reviewed changes

openjdk bot added ready and removed csr labels Dec 2, 2024

openjdk bot added the integrated label Dec 3, 2024

openjdk bot closed this Dec 3, 2024

openjdk bot removed ready rfr labels Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear #22454

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear #22454

jaikiran commented Nov 29, 2024 •

edited by openjdk bot

Loading

jaikiran commented Nov 29, 2024

bridgekeeper bot commented Nov 29, 2024

openjdk bot commented Nov 29, 2024 •

edited

Loading

openjdk bot commented Nov 29, 2024

openjdk bot commented Nov 29, 2024

mlbridge bot commented Nov 29, 2024 •

edited

Loading

dfuch left a comment

jaikiran commented Nov 29, 2024

Michael-Mc-Mahon Nov 29, 2024

jaikiran Nov 29, 2024

dfuch left a comment

Michael-Mc-Mahon left a comment

jaikiran commented Dec 3, 2024

jaikiran commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear #22454

8235786: Javadoc for com/sun/net/httpserver/HttpExchange.java#setAttribute is unclear #22454

Conversation

jaikiran commented Nov 29, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

jaikiran commented Nov 29, 2024

bridgekeeper bot commented Nov 29, 2024

openjdk bot commented Nov 29, 2024 • edited Loading

openjdk bot commented Nov 29, 2024

openjdk bot commented Nov 29, 2024

mlbridge bot commented Nov 29, 2024 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

jaikiran commented Nov 29, 2024

Michael-Mc-Mahon Nov 29, 2024

Choose a reason for hiding this comment

jaikiran Nov 29, 2024

Choose a reason for hiding this comment

dfuch left a comment

Choose a reason for hiding this comment

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

jaikiran commented Dec 3, 2024

jaikiran commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

openjdk bot commented Dec 3, 2024

jaikiran commented Nov 29, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 29, 2024 •

edited

Loading

mlbridge bot commented Nov 29, 2024 •

edited

Loading