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

8254967: com.sun.net.HttpsServer spins on TLS session close #742

Closed

Conversation

Michael-Mc-Mahon
Copy link
Member

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

This fixes a busy loop bug in the Http server which happens sometimes when an SSL connection is closed by the client.

There is no regression test as it is not easy to reproduce and the only effect is that one executor thread gets tied up.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) (2/9 running) (9/9 running) ✔️ (9/9 passed)

Issue

  • JDK-8254967: com.sun.net.HttpsServer spins on TLS session close

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/742/head:pull/742
$ git checkout pull/742

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 19, 2020

👋 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 openjdk bot added the rfr label Oct 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@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 label Oct 19, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 19, 2020

Webrevs

handshaking.lock();
ByteBuffer tmp = allocate(BufType.APPLICATION);
WrapperResult r;
Status st;
HandshakeStatus hs;
do {
tmp.clear();
tmp.flip ();
r = wrapper.wrapAndSendX (tmp, true);
} while (r.result.getStatus() != Status.CLOSED);
hs = r.result.getHandshakeStatus();
st = r.result.getStatus();
} while (st != Status.CLOSED &&
!(st == Status.OK && hs == HandshakeStatus.NOT_HANDSHAKING));
} finally {
handshaking.unlock();
}
Copy link
Member

@dfuch dfuch Oct 19, 2020

Choose a reason for hiding this comment

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

Ok. The expectation is that doClosure() only involves sending a close acknowedged and not receiving anything. This is probably correct if doClosure() is called on reception of close_notify. In which case the change looks reasonable. I also see that a further expectation is that cycles of NEED_WRAP/NEED_UNWRAP only happen during the handshake, which is hopefully true.

dfuch
dfuch approved these changes Oct 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@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:

8254967: com.sun.net.HttpsServer spins on TLS session close

Reviewed-by: dfuchs

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

  • 1da28de: 8255009: delta apply fixes for JDK-8246774 and JDK-8253455, pushed too soon
  • a0382cd: 8253235: JFR.dump does not respect maxage parameter
  • cb7701b: 8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)
  • 4ffed32: 8254940: AArch64: Cleanup non-product thread members
  • cd66e0f: 8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message
  • e76de18: 8223051: support loops with long (64b) trip counts
  • e9be2db: 8253453: SourceFileInfoTable should be allocated lazily
  • e10b538: 8243962: Various JVM TI tests time out using JFR on Windows
  • a1a3e9d: 8254882: ZGC: Use static_assert instead of guarantee
  • 8edc2f0: 8254912: ZGC: Change ZCollectionInterval type to double
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/919978380c14fc44961c4a0e52e442e072108d03...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 label Oct 19, 2020
@Michael-Mc-Mahon
Copy link
Member Author

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

/integrate

@openjdk openjdk bot closed this Oct 19, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 19, 2020

@Michael-Mc-Mahon Since your change was applied there have been 188 commits pushed to the master branch:

  • 1da28de: 8255009: delta apply fixes for JDK-8246774 and JDK-8253455, pushed too soon
  • a0382cd: 8253235: JFR.dump does not respect maxage parameter
  • cb7701b: 8253970: Build error: address argument to atomic builtin must be a pointer to integer or pointer ('volatile narrowOop *' invalid)
  • 4ffed32: 8254940: AArch64: Cleanup non-product thread members
  • cd66e0f: 8253877: gc/g1/TestGCLogMessages.java fails - missing "Evacuation failure" message
  • e76de18: 8223051: support loops with long (64b) trip counts
  • e9be2db: 8253453: SourceFileInfoTable should be allocated lazily
  • e10b538: 8243962: Various JVM TI tests time out using JFR on Windows
  • a1a3e9d: 8254882: ZGC: Use static_assert instead of guarantee
  • 8edc2f0: 8254912: ZGC: Change ZCollectionInterval type to double
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/919978380c14fc44961c4a0e52e442e072108d03...master

Your commit was automatically rebased without conflicts.

Pushed as commit 953e472.

💡 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 JDK-8254967 branch Oct 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated net
2 participants