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

8308593: Add KEEPALIVE Extended Socket Options Support for Windows #2278

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tkyc
Copy link

@tkyc tkyc commented Nov 10, 2023

This is an unclean backport of JDK-8308593, which provides support for the keepalive extended socket options on Windows.

The following changes are the differences from the original patch in order to get this backport working for 11u:

make/lib/Lib-jdk.net.gmk -- Updated this script to support building extnet for Windows.

src/jdk.net/share/classes/jdk/net/ExtendedSocketOptions.java -- Added a case to support returning an instance of WindowsSocketOptions in PlatformSocketOptions.

src/java.base/windows/classes/java/net/PlainSocketImpl.java -- The Windows PlainSocketImpl class will need to have similar setOption/getOption methods as the Unix PlainSocketImpl class to support setting these extended socket options for Windows.

Backport passed against Tier 1 tests and was manually verified to be working on Windows.


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
  • JDK-8308593 needs maintainer approval

Issue

  • JDK-8308593: Add KEEPALIVE Extended Socket Options Support for Windows (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2278/head:pull/2278
$ git checkout pull/2278

Update a local copy of the PR:
$ git checkout pull/2278
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2278/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2278

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2278.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 10, 2023

👋 Welcome back tkyc! 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 openjdk bot changed the title Backport f3ade388dac0b882e671462caa762138f44817fb 8308593: Add KEEPALIVE Extended Socket Options Support for Windows Nov 10, 2023
@openjdk
Copy link

openjdk bot commented Nov 10, 2023

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Nov 10, 2023
@tkyc
Copy link
Author

tkyc commented Nov 10, 2023

/approval request Requesting backport approval to 11u to have keepalive parity with the other OS platforms. Risk is low as the new code path in the backported code applies to Windows only and requires the new socket options to be set to take effect. Backport was ran against Tier 1 tests and was also manually tested. There's also existing tests in 11u that cover this scenario and so no new tests were added.

@openjdk
Copy link

openjdk bot commented Nov 10, 2023

@tkyc
8308593: The approval request has been created successfully.

@openjdk openjdk bot added the approval label Nov 10, 2023
@mlbridge
Copy link

mlbridge bot commented Nov 10, 2023

Webrevs

@GoeLin
Copy link
Member

GoeLin commented Nov 13, 2023

Hi @tkyc,
please only label for approval if you have a review, i.e. the change is ready to be integrated.
Did you make sure all relevant tests did run? Tier1 is only a basic sanity check.

@openjdk openjdk bot removed the approval label Nov 13, 2023
@tkyc
Copy link
Author

tkyc commented Nov 15, 2023

@GoeLin apologies, my mistake. Aside from tier 1, I ran the keepalive tests from PR JDK-8194298 that initially added support for Linux (which passed). At the moment also I'm running the jdk_net test group as well just as another precaution. Let me know if there's any further testing I can do.

@tkyc
Copy link
Author

tkyc commented Nov 15, 2023

An update, my jdk_net test group run also passed.

@tkyc
Copy link
Author

tkyc commented Nov 24, 2023

Hello, let me know if there's anything else I can do to move this forward.

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 23, 2023

@tkyc This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@tkyc
Copy link
Author

tkyc commented Dec 24, 2023

Hello, just bumping the thread. Let me know if there's anything I can do to progress this towards integration. I'll check back in the new year as right now I'm on vacation.

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 21, 2024

@tkyc This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@tkyc
Copy link
Author

tkyc commented Jan 22, 2024

Let me know how I can progress this towards integration. Thanks.

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 19, 2024

@tkyc This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
2 participants