8298588: WebSockets: HandshakeUrlEncodingTest unnecessarily depends on a response body#11486
8298588: WebSockets: HandshakeUrlEncodingTest unnecessarily depends on a response body#11486Karm wants to merge 3 commits intoopenjdk:masterfrom
Conversation
According to [rfc6455](https://www.rfc-editor.org/rfc/rfc6455.html#section-5.5.1), the close frame MAY contain a body, i.e. it is considered [optional](https://www.rfc-editor.org/rfc/rfc2119#section-5). It seems that the contemporary JDK HEAD (tip) does populate the body and thus enables `HandshakeUrlEncodingTest.java` to parse its contents. On the contrary, JDK 11 does not populate the body in the same case. I would like to backport JDK-8245245 all the way to JDK 11, so I would like to change the behavior of this test so as it works across JDK versions.
|
👋 Welcome back Karm! A progress list of the required criteria for merging this PR into |
|
Hello @Karm,
I believe the right thing to do here, to help with the backport of this test, is to also backport the actual change/fix which populates the |
Maybe. On the other hand, adding the response to the body of a websocket request is a behaviour change, which I find questionable as to being "the right thing to do", considering we are trying to solve an encoding bug in JDK 11 in a stable release. A bug fix shouldn't have a behaviour changing enhancement as a dependency if at all possible. Would this patch not be acceptable as is? It only encodes what the RFC states (allowing for empty body responses). Would the patch be more amenable if it were add additional body assertions iff the body is present? |
|
Hello Severin,
I see what you mean. In that case I think the backport process allows for the test to be updated in a way that it verifies (and passes) the actual bug fix i.e. you could do what you are proposing here, in the backport patch against the JDK 11 backport itself. My understanding is that the mainline isn't expected to take in such changes since the mainline isn't expected to be JDK version agnostic. Having said that I think someone who's more aware of these processes will be able to provide a definitive answer. So I would recommend bringing this up in the net-dev mailing list https://mail.openjdk.org/mailman/listinfo/net-dev (this current PR hasn't triggered the official review process because of which the conversation here hasn't been copied over to the mailing list by the bots). |
|
@Karm I've created https://bugs.openjdk.org/browse/JDK-8298588 for this test update. Please change the PR title to |
Karm
changed the title
Webrevs
|
dfuch
left a comment
dfuch
left a comment
There was a problem hiding this comment.
Checking the response URI is good but for the mainline I'd advise to check both the response URI and the body. It's fine to not check the response body on the update releases where JDK-8240666 is not present.
Whether you want to backport JDK-8240666 or not is an independent and unrelated decision - IMO.
Thank you for the comment @dfuch My objective is to have JDK-8245245 fixed in JDK 11. I cannot do that cleanly because if I just took the original patch unchanged, HandshakeUrlEncodingTest would start to fail as it depends on JDK-8240666. I was advised that is it not the best practice to alter something, i.e. HandshakeUrlEncodingTest, just in JDK 11 without doing it on the tip (mainline) first. I don't think it's justifiable to backport JDK-8240666 solely to be able to cleanly backport JDK-8245245 as those two issues are only superficially related as I see it. Could you help me put this in the context of our commentary on openjdk/jdk11u-dev#1558 (comment) and to plot a course of action that would be suitable, please? |
|
@Karm Have you considered my suggestion from earlier? |
I missed that in the comments. It makes perfect sense to me. I'll amend the patch this way. |
There was a problem hiding this comment.
In the case of the mainline there MUST be a body - otherwise that is a bug in the implementation.
I believe the mainline test should therefore asserts that the body is present and equals to what it expects.
There is no requirement that a test in the mainline be the same than in previous releases, especially when the mainline test makes use of a functionality that is not present in those releases.
It should be perfectly fine to have the mainline test checks two things, and the update release test only one thing, because the second thing is not implemented in the update release.
If you modify the mainline test to simply add a check to assert that the response URI is what the text expects, then the diff between the update release test (that only check the response URI) and the mainline test after your fix (that checks both the response URI and the body) should naturally show that the mainline test has one additional test compared to the update release test - which is making use of a functionality only available in the mainline.
I would even suggest to modify the handler in the update release to not write anything into the response body since the body is discarded by the client implementation in that release.
The reason I'm a bit insistent on keeping the check on the body string is that it's the only thing that can reliably assert that the server has received the correct URI. Conceptually, the URI in the HttpResponse could have be composed by the client implementation itself. Writing the URI in the response body was a convenient way to check what the server had actually received.
There was a problem hiding this comment.
Thx @dfuch for the comment.
In the case of the mainline there MUST be a body - otherwise that is a bug in the implementation. I believe the mainline test should therefore asserts that the body is present and equals to what it expects.
I can see your point and I changed the test in my latest commit. The reason for my original approach for omitting it and for the subsequent step of checking for body presence was that the behavior of not/dropping body has its own test in WSHandshakeExceptionTest.java.
There is no requirement that a test in the mainline be the same than in previous releases, especially when the mainline test makes use of a functionality that is not present in those releases.
I see. I might got confused by comments on the jdk11u-dev thread:
The thing to do is use the buggy code from the original commit in this backport,
fix the problem in tip, then backport that fix to 19u, 17u, and 11u.
It should be perfectly fine to have the mainline test checks two things, and the update release test only one thing, because the second thing is not implemented in the update release.
If you modify the mainline test to simply add a check to assert that the response URI is what the text expects, then the diff between the update release test (that only check the response URI) and the mainline test after your fix (that checks both the response URI and the body) should naturally show that the mainline test has one additional test compared to the update release test - which is making use of a functionality only available in the mainline.
I would even suggest to modify the handler in the update release to not write anything into the response body since the body is discarded by the client implementation in that release.
The diff between the test in this PR (tip, mailine) and the one in JDK 11 one pull/1558 is https://editor.mergely.com/dOyW1m6w/ .
The reason I'm a bit insistent on keeping the check on the body string is that it's the only thing that can reliably assert that the server has received the correct URI. Conceptually, the URI in the HttpResponse could have be composed by the client implementation itself. Writing the URI in the response body was a convenient way to check what the server had actually received.
I am glad you are providing feedback.
dfuch
left a comment
dfuch
left a comment
There was a problem hiding this comment.
This looks good to me! Thanks for improving this test. Please double check it still passes before integrating :-)
|
@Karm 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: 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 222 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dfuch) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
It Works On My Machine ™️ |
|
/integrate |
|
I've run these changes against our internal CI systems and the tests went fine. |
|
/sponsor |
|
Going to push as commit 909d0cb.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
4 participants
According to rfc6455, the close frame MAY contain a body, i.e. it is considered optional. It seems that the contemporary JDK HEAD (tip) does populate the body and thus enables
HandshakeUrlEncodingTest.javato parse its contents. On the contrary, JDK 11 does not populate the body in the same case. I would like to backport JDK-8245245 all the way to JDK 11, so I would like to change the behavior of this test so as it works across JDK versions.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11486/head:pull/11486$ git checkout pull/11486Update a local copy of the PR:
$ git checkout pull/11486$ git pull https://git.openjdk.org/jdk pull/11486/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 11486View PR using the GUI difftool:
$ git pr show -t 11486Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11486.diff