Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8288493: Document JDK specific system properties in jdk.httpserver #10766

Closed

Conversation

Michael-Mc-Mahon
Copy link
Member

@Michael-Mc-Mahon Michael-Mc-Mahon commented Oct 19, 2022

Hi,

This change adds some documentation to the jdk.httpserver module-info listing all of the system properties used by the implementation and which can be set by users.

CSR at https://bugs.openjdk.org/browse/JDK-8295667

Thanks,
Michael.


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 a CSR request to be approved

Issues

  • JDK-8288493: Document JDK specific system properties in jdk.httpserver
  • JDK-8295667: Document JDK specific system properties in jdk.httpserver (CSR)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10766

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 19, 2022

👋 Welcome back michaelm! 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
Copy link

openjdk bot commented Oct 19, 2022

@Michael-Mc-Mahon 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.

@openjdk openjdk bot added the net net-dev@openjdk.org label Oct 19, 2022
* Boolean value, which if true, generates debug information on the console.
* </li>
* <li><p><b>{@systemProperty sun.net.httpserver.nodelay}</b> (default: false)<br>
* Boolean value, which if true, sets the TCP_NODELAY socket option on all incoming connections.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe link it to TCP_NODELAY socket option here? {@link java.net.SocketOptions#TCP_NODELAY}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{@link java.net.SocketOptions#TCP_NODELAY}

or {@link java.net.StandardSocketOptions#TCP_NODELAY} - it has better docs

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the second one is better

@Michael-Mc-Mahon Michael-Mc-Mahon marked this pull request as ready for review October 19, 2022 16:01
@openjdk openjdk bot added rfr Pull request is ready for review csr Pull request needs approved CSR before integration labels Oct 19, 2022
@mlbridge
Copy link

mlbridge bot commented Oct 19, 2022

@openjdk openjdk bot removed the rfr Pull request is ready for review label Oct 20, 2022
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 20, 2022
* <li><p><b>{@systemProperty sun.net.httpserver.maxIdleConnections}</b> (default: 200)<br>
* The maximum number of idle connections at a time.
* </li>
* <li><p><b>{@systemProperty sun.net.httpserver.drainAmount}</b> (default: 64K)<br>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Michael, sorry I missed this in my previous review. The value for drainAmount is expected to be a long value. So I think the default value here should be 65536. Readers might take the 64K value literally and might attempt to pass similar values to this property which will then lead to failures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we additionally have to specify the behaviour when the value is <=0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I can change the 64K to 65536. I think the spec already covers the <=0 case. Thanks!

* The maximum time in milliseconds allowed to receive a request headers and body.
* In practice, the actual time is a function of request size, network speed, and handler
* processing delays. A value less than or equal to zero means the time is not limited.
* If the limit is exceeded then the connection is terminated and the handler will receive a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This states the handler will receive a IOException. Should that instead say the client will receive a IOException? Same with the text in maxRspTime.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I was referring to the HttpHandler here on the server side.

@Michael-Mc-Mahon
Copy link
Member Author

The next update I'm going to push includes doc changes that describe what happens when potentially invalid/unexpected values (eg negative numbers or garbage strings) are specified for the system properties. Checking this showed the behavior of the property "sun.net.httpserver.maxReqHeaders" with a value <=0 doesn't make sense and would end up rejecting all requests. So, I'm specifying and changing the implementation so such values result in the default value being used. I will update the CSR to reflect the behavior change also.

Copy link
Member

@jaikiran jaikiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes, Michael. The latest update looks fine to me, both the source code change and the text in the module-info.java.

Copy link
Member

@AlekseiEfimov AlekseiEfimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest changes looks good to me too.

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Oct 21, 2022
@openjdk
Copy link

openjdk bot commented Oct 21, 2022

@Michael-Mc-Mahon 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:

8288493: Document JDK specific system properties in jdk.httpserver

Reviewed-by: dfuchs, jpai, aefimov

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 42 new commits pushed to the master branch:

  • 0c13d66: 8295530: Update Zlib Data Compression Library to Version 1.2.13
  • 15bebf9: 8295666: Linux x86 build fails after 8292591
  • 5064718: 8294460: CodeSection::alignment checks for CodeBuffer::SECT_STUBS incorrectly
  • 8b010e0: 8030616: sun/management/jmxremote/bootstrap/RmiBootstrapTest fails intermittently with cannot find a free port
  • b35922b: 8295714: GHA ::set-output is deprecated and will be removed
  • dfd2d83: 8295657: SA: Allow larger object alignments
  • a345df2: 8280131: jcmd reports "Module jdk.jfr not found." when "jdk.management.jfr" is missing
  • ef62b61: 8295703: RISC-V: Remove implicit noreg temp register arguments in MacroAssembler
  • 6240431: 8295697: Resolve conflicts between serviceability/jvmti and nsk/jvmti shared code
  • 1164258: 8295124: Atomic::add to pointer type may return wrong value
  • ... and 32 more: https://git.openjdk.org/jdk/compare/857b0f9b05bc711f3282a0da85fcff131fffab91...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 openjdk bot added the ready Pull request is ready to be integrated label Oct 21, 2022
@Michael-Mc-Mahon
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Oct 24, 2022

Going to push as commit 5596d9a.
Since your change was applied there have been 57 commits pushed to the master branch:

  • 7a49c9b: 8295717: Minimize disabled warnings in accessibility native code
  • 7460661: 8294466: Minimize disabled warnings in java.desktop
  • 89a94d0: 8293873: Centralize the initialization of UL
  • 08d3ef4: 8295413: com/sun/jdi/EATests.java fails with compiler flag -XX:+StressReflectiveCode
  • 65c84e0: 8295777: java/net/httpclient/ConnectExceptionTest.java should not rely on system resolver
  • 329b49a: 8266900: java/net/httpclient/ShortResponseBody.java fails on windows with java.io.IOException: Unable to establish loopback connection
  • aad81f2: 8293979: Resolve JVM_CONSTANT_Class references at CDS dump time
  • 7cbf672: 8295811: serviceability/sa/TestObjectAlignment.java fails on x86_32
  • adad59e: 8295762: [Vector API] Update generate_iota_indices for x86_32 after JDK-8293409
  • b5efa2a: 8294538: missing is_unloading() check in SharedRuntime::fixup_callers_callsite()
  • ... and 47 more: https://git.openjdk.org/jdk/compare/857b0f9b05bc711f3282a0da85fcff131fffab91...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Oct 24, 2022
@openjdk openjdk bot closed this Oct 24, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 24, 2022
@openjdk
Copy link

openjdk bot commented Oct 24, 2022

@Michael-Mc-Mahon Pushed as commit 5596d9a.

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

@Michael-Mc-Mahon Michael-Mc-Mahon deleted the httpserverprops branch October 24, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated net net-dev@openjdk.org
4 participants