8349670: HttpServer: sending interim responses fails after JDK-7026262

[SentryMan]

Following the guideline of the last comment on JDK-8349670, resolves the issue where sending a 1xx status code would close the input stream, preventing the server from reading the body.

• When a 1xx status code is sent by sendResponseHeaders, the input/output streams will not be closed prematurely.
• sentHeaders will not be set to true when sending 1xx status codes
• 100-continue will be sent automatically when trying to read the inputstream if Expect: 100-continue header is present

---

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

Issue

• JDK-8349670: HttpServer: sending interim responses fails after JDK-7026262 (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27069

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

Hi @SentryMan, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user SentryMan" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@SentryMan 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]

@SentryMan Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[SentryMan]

/signed

[SentryMan]

It turns out I had previously signed the OCA but forgot to put my GH username on it.

Edit: no wait my OCA does have my GH username on it?

[bridgekeeper]

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

[mlbridge]

Webrevs

• 20: Full - Incremental (b56541c4)
• 19: Full - Incremental (071a7668)
• 18: Full - Incremental (52fc572c)
• 17: Full - Incremental (f9dc2650)
• 16: Full - Incremental (e1ce09d9)
• 15: Full - Incremental (9d3f0943)
• 14: Full - Incremental (c8fbcba2)
• 13: Full - Incremental (84aebe9b)
• 12: Full - Incremental (2afaeb89)
• 11: Full - Incremental (8cc60c69)
• 10: Full - Incremental (8abfba3a)
• 09: Full - Incremental (4b005888)
• 08: Full - Incremental (ca50648e)
• 07: Full - Incremental (144110bf)
• 06: Full - Incremental (07282362)
• 05: Full - Incremental (2f969128)
• 04: Full - Incremental (b41f1dc1)
• 03: Full - Incremental (f6a5a224)
• 02: Full - Incremental (fa42e77c)
• 01: Full - Incremental (d76ff991)
• 00: Full (bbc2a975)

[djelinski]

Hi @SentryMan, thanks for contributing!

The informational response handling looks good, but the upgrades need to be handled differently. If we start treating every request with an Upgrade header as an upgrade, we will invalidate pretty much every existing application that uses the httpserver. We need a check that the application understands the semantics of the Upgrade header before we proceed with the upgrade.

Both uses of the Upgrade header (WebSockets and HTTP/2 upgrade) provide means for the server to accept or reject the upgrade. In both cases the server is required to send a 101 Switching protocols response, any other response means that the server decided to keep using HTTP 1.1.

The second issue is resource handling. The server keeps a list of all active connections and exchanges. The existing OutputStream classes like UndefLengthOutputStream notify the server when the exchange finishes, permitting it to close the exchange and remove the connection from the active connection list as needed. Here you give the raw input/output streams to the user; the raw streams do not notify the server, so the server's bookkeeping will be incorrect.

[SentryMan]

Thank you for taking the time to review.

In both cases the server is required to send a 101 Switching protocols response, any other response means that the server decided to keep using HTTP 1.1.

It does not appear that this is the case. Looking at rfc9110, a server can send other status codes to reject the connection, such as 426 or ignore the header altogether and not upgrade as you say.

[dfuch]

Hi Josiah! Glad to see that the OCA verification finally went through!

It seems to me that this PR addresses two different problems:

1. attempting to fix sendResponseHeaders to support sending informational responses (1xx status - a bug fix)
2. attempting to add support for switching protocols to the HttpServer (a new functionality)

I believe it would be best to separate these two issues into two different PRs to avoid mixing them up.

I would suggest only addressing 1. in this PR and log an enhancement for 2.

With regard to supporting some way to support upgrading protocols in the HTTP server I believe more discussion will be needed.

best regards, -- daniel

[SentryMan]

ok will move the upgrade stuff to a different pr

[Michael-Mc-Mahon]

I have to say I'm a bit uncomfortable trying to shoe-horn all this functionality into the existing method sendResponseHeaders. There are significant behavioral implications for the server with each of the 1XX intermediate response codes. Take 103 early Hints for example. That is usually sent with a Link header in the intermediate response. Where would the header come from? If it's the response headers returned from getResponseHeaders then is this map cleared after the intermediate response or is the user supposed to clear it? Does this work?

If we are supporting 100 Continue now (under user control) does the expected behavior work? If a client sends a request with an Expect: 100-Continue header and waits for the intermediate response before sending the request body, does this actually work? I know we have partial support for it already. But, I'd like to see tests and apidoc documenting all of this before we consider adding it.

[dfuch]

@Michael-Mc-Mahon are you suggesting we should add a new API to send informational codes instead, and throw in sendResponseHeaders() if the status is >= 100 and < 200?

[Michael-Mc-Mahon]

@Michael-Mc-Mahon are you suggesting we should add a new API to send informational codes instead, and throw in sendResponseHeaders() if the status is >= 100 and < 200?

That was my initial expectation. I was expecting a new method like sendIntermediateResponse(int code, Headers headers)

If we're not adding a new API, then I think all the above questions need answers and the spec for sendResponseHeaders updated to spell out how all this functionality would work, with tests showing it. Either way, I think this change needs a CSR.

I suppose also the question of whether or not to add a new method depends on to what extent this actually worked (even if it was never intended that it should work) before 7026262

[SentryMan]

103

103 is for http2, and a lot of clients won't even accept it if the connection is not on http2

If it's the response headers returned from getResponseHeaders then is this map cleared after the intermediate response or is the user supposed to clear it? Does this work?

alright going back to 101 for a second, there is no difficulty, as the 101 is the final response with the final headers, rather than other codes. I can toss this PR and get back exclusively to 101 if we decide the effects for 100-continue are undesirable.

If we are supporting 100 Continue now (under user control) does the expected behavior work? If a client sends a request with an Expect: 100-Continue header and waits for the intermediate response before sending the request body, does this actually work?

We removed that behavior from this PR, but while it was in here (4b00588), it worked as expected, the client waited for the manually sent 100 code to send the body and everything.

[SentryMan]

We removed that behavior from this PR

@Michael-Mc-Mahon just to give a recap, right now this PR doesn't allow control over 100-continue, it just allows you to send extra superfluous 100 (because the current logic where we auto-send 100 continue before the handler is still intact). It was determined that moving the auto continue logic to when the inputstream is read should be evaluated as a separate issue.

[SentryMan]

If it's the response headers returned from getResponseHeaders then is this map cleared after the intermediate response or is the user supposed to clear it? Does this work?

It works either way, but clearing I feel is unexpected and undesirable

[SentryMan]

It occurs to me, that there is actually no practical reason for this PR. For the 100-continue case, there are better ways to handle it than sendResponseHeaders or even a new public sendIntermediateResponse method. The best method I think would be to be like jetty and move the auto-send-continue logic to when the inputstream is read. This is because trying to read the input stream implies that you're okay to receive the body.

For connection upgrades, this PR is also unrelated as besides keeping the inputstream open, it otherwise behaves like other status codes and doesn't require sending 101 and then some other status.

[dfuch]

I'm personally OK with adding a new API to send 1xx response.

WRT 100 I'm a bit of two minds with how to handle that. We heavily use the HttpServer to test the HttpClient HTTP/1.1 stack. In the Httpclient, we wanted to test that if the server didn't send 100, the client would send the body after a reasonable timeout. Then we wanted to test that the 100 would be ignored if it came after the client had started to send the body. We also wanted to test that additional 100 would be ignored.
At the moment we need to use a ServerSocket on the server side to do that for HTTP/1.1 because the HttpServer doesn't offer this level of granularity. Adding auto-send in getRequestBody().read() would not allow us to use the server to do that either.
Another thing we wanted to test is that the client would ignore 1xx code it does not know how to handle.
I was hopping that a method to send informational response codes (wether with sendResponseHeaders or with a new API) would help us with testing these various scenarios.
At the same time - we want our API to be as user friendly as possible - so ability to use the server to test complex scenarios for the benefit of the client comes second. Though equally, maybe other people use our server to test their own client implementations?
So that's to explain where I'm coming from.

[SentryMan]

We heavily use the HttpServer to test the HttpClient HTTP/1.1 stack. In the Httpclient, we wanted to test that if the server didn't send 100, the client would send the body after a reasonable timeout. Then we wanted to test that the 100 would be ignored if it came after the client had started to send the body. We also wanted to test that additional 100 would be ignored.

Adding auto-send in getRequestBody().read() allows this though? put a sleep in the handler before getRequestBody().read(). You have control if/when the 100 is sent, no new public method required.

[dfuch]

Adding auto-send in getRequestBody().read() allows this though? put a sleep in the handler before getRequestBody().read(). You have control if/when the 100 is sent, no new public method required.

The way we would do this with our HTTP/2 test server is to consume the first byte of the request body before sending 100, so as not send 100 before having received the first byte of the body.

[robaho]

The way we would do this with our HTTP/2 test server is to consume the first byte of the request body before sending 100

That is invalid. The whole purpose is to enable the client to avoid sending the request body until the 100 Continue is received (i.e all headers validated by the handler).

[SentryMan]

That is invalid. The whole purpose is to enable the client to avoid sending the request body until the 100 Continue is received (i.e all headers validated by the handler).

they're testing the client not the server, but it is a bit strange indeed since the client is already in the process of sending the body.

Another thing we wanted to test is that the client would ignore 1xx code it does not know how to handle.

You can send 1xx codes today with sendResponseHeaders, the only caveat is that you can't read the InputStream anymore.

[dfuch]

That is invalid. The whole purpose is to enable the client to avoid sending the request body until the 100 Continue is received (i.e all headers validated by the handler).

Well - this HTTP/2 server is a test server - and it's useful to be able to do "invalid" things in order to test how the client reacts. And note that this is not strictly invalid, since from the client standpoint - it looks exactly the same as if the handling of the request had been delayed on the server side. The client has no way to know that the server has consumed the first byte.

[SentryMan]

In any case, no matter what option we go with to support 100-continue the automatic sending logic present now will have to be moved/modified as adding a new method doesn't change the fact that 100-continue is being sent before handlers are called. Perhaps we should focus more on that part before we add new methods that don't actually change the situation

[dfuch]

In any case, no matter what option we go with to support 100-continue the automatic sending logic present now will have to be moved/modified as adding a new method doesn't change the fact that 100-continue is being sent before handlers are called. Perhaps we should focus more on that part before we add new methods that don't actually change the situation

True. But it would at least allow us to test that status code 123 is properly discarded.

[SentryMan]

True. But it would at least allow us to test that status code 123 is properly discarded.

you can do this now: exchange.sendResponseHeaders(123, -1)

[robaho]

@dfuch it is invalid in the client, the client if it sets the Expect 100-Continue should not send the body until it receives the 100 Continue. See the rfc.

[SentryMan]

@dfuch it is invalid in the client, the client if it sets the Expect 100-Continue should not send the body until it receives the 100 Continue. See the rfc.

okay they should but the rfc also allows for the client to send the body before receiving 100 because servers might ignore the Expect header

[robaho]

@SentryMan you are correct. The rfc confirms it - sorry for the noise @dfuch

[dfuch]

you can do this now: exchange.sendResponseHeaders(123, -1)

You mean - with the changes in your PR? Because without these changes I suspect you might have to pass 0, not -1. So pretend that there is a body of unknown length.

[SentryMan]

Because without these changes I suspect you might have to pass 0, not -1. So pretend that there is a body of unknown length.

without the changes it's treated like a response without a body, so 0/-1 doesn't matter as it will be overridden

[Michael-Mc-Mahon]

It occurs to me, that there is actually no practical reason for this PR. For the 100-continue case, there are better ways to handle it than sendResponseHeaders or even a new public sendIntermediateResponse method. The best method I think would be to be like jetty and move the auto-send-continue logic to when the inputstream is read. This is because trying to read the input stream implies that you're okay to receive the body.

For connection upgrades, this PR is also unrelated as besides keeping the inputstream open, it otherwise behaves like other status codes and doesn't require sending 101 and then some other status.

Possibly. If we are agreed that 100 Continue is the only potential practical use case for this (other than 101 which is out of scope), then we definitely don't need a new method at this time, and we then need to ask what point is there in sending a 100 Continue considering that the server already handles it. Is there a use case for allowing the user to send it separately? If not, I'm questioning the benefit of allowing it. As I said from the start, this was never intended to be supported in the initial implementation and my first thought was just to disallow it formally.

[dfuch]

If we're not going to allow sendResponseHeaders to accept 1xx codes and wait for a final response then I believe we should modify it to throw if a 1xx status code is passed, and document that formally in the API documentation of the method, rather than have the implementation send 1xx as a final response. Maybe this PR should do that. We could simply throw if other invalid codes are passed (anything < 200 or > 999). Let's wait until we get consensus before adding new commits though.

[SentryMan]

Possibly. If we are agreed that 100 Continue is the only potential practical use case for this

as the PR stands now, even that is no longer relevant, because of the auto-send.

We are in one accord then that this PR implementation does not have anything of worth.

Continue considering that the server already handles it. Is there a use case for allowing the user to send it separately?

Given that the server automatically sends 100-Continue without allowing the handler to validate if the body should be sent, I think we should make a different PR delaying the auto send until getRequestBody().read(). This would allow for proper 100 continue behavior where the server only continues the request if the request is validated.

If we're not going to allow sendResponseHeaders to accept 1xx codes

I'm amenable to banning 100 (because the server handles it) and 103 (needs http2), but once again I ask for a discussion on http 101 before we proceed to say that all 1xx codes are not accepted.

I've created #27751 for review and a place to discuss, would you kindly take a moment to review and voice your concerns?

[SentryMan]

closing as there are better ways to handle 100-continue