GeezerVison

I really need a bug that describes the problem in detail. All of the core JDK tests pass with the current code so I need to better understand the problem this is addressing and why

I looked into this a bit. I think the framework needs to be the one sending 100 level responses in some very specific circumstances. The proper place to discuss this is in an issue and not the PR.

https://bugs.openjdk.org/browse/JDK-8349670

You’ll have to be more for specific. I don’t see how that relates to this PR. The bug you reference doesn’t discuss 1xx returns in the problem report. There are RFCs that govern what should happen here. We could have an enhancement to support ‘expect continue’ header.

The comments on the bug state that sending interim responses is an anti pattern. I suspect this is something that cannot be changed in order to remain compatible I do think supporting 100 Continue is possible.

Essentially, calling sendheaders twice doesn't work. So you are unable to send a 1xx code using sendheaders, and then call it again to send a 200 for the final response.

The comments on the bug state that sending interim responses is an anti pattern.

Read the more recent comments on the issue. or even the PR that's open to fix it

Please add a compliant test case to the PR - preferably the one from the JDK that tests this. The bug is not fixed and I suspect it is because it breaks compatibility and/or other current documented behavior.

This way I can run the test case against the JDK version as well.

Took a test from the PR to fix on the JDK

Hi, I made some comments on the JDK issue regarding the test. openjdk/jdk#27069 (comment)

I think the we need that to ensure things are working as expected.

I am also not certain what the JDK folks are going to decide. I looked at Jetty and it automatically sends the '100 Continue' on the getInputStream() call by the handler. Maybe the exchange needs to track if the continue has been sent already, and if not send it, or we can punt to the manual method your code uses.

Maybe the exchange needs to track if the continue has been sent already, and if not send it, or we can punt to the manual method your code uses.

I mean that's cool and all, but the specification says

A server MAY omit sending a 100 (Continue) response if it has already received some or all of the content for the corresponding request, or if the framing indicates that there is no content.

right, so you need to send the 100 Continue if it hasn't received any of the content. The purpose is to enable the sender to not send the body if the request is going to fail. If your test case ensured this - it would hang - that is it expects the continue.

100% that it is a bug in the JDK and the roboho version that neither implements the handling of 'Expect: 100-Continue'.

I believe for consistency it is probably better to have the server automatically respond when the handler attempts to read the input stream, or you will get duplicate '100 Continue' messages - which will break the client reading the response.

To clarify, if I don't do it in the server, technically every handler must understand 'Expect: 100-Continue' and handle it or the clients will hang.

I've reviewed and tested the patch and there is another issue. The content-length should really only be set to 0 when switching protocols - and I'm not even sure that is right - it was done solely to enable the websocket approach. Additionally, in the same vein as the "sending the headers twice" the content length is sent with the informational response. This would incorrect (assuming so) in the case of a 100 Continue, because you would send a content length of zero, then in the full response you may have a non zero content length.

So if I pass the -1, then the exchange will be closed, which isn't right. So I think it needs more special case handling.

I am starting to think this is attempting to shoehorn something in (connection upgrades) which the JDK was not designed to support generally. And it really needs this support, and general support for the other interim responses.

The other major problem, is that calling setResponseHeaders is what sets up the linked output streams, which is also why you are not supposed to call this more than once.

If code was not properly ordered, like first get the input stream, then send the informational then send the response - it would all break.

It is also why there is the 'websocket' flag in the exchange, due to this support - in order to hand the request over. It seems that maybe I could change this to be an 'upgrade connection' flag at this time, to make switching protocols support more generic.

I made some changes and the tests pass, but I am still unclear as to why you needed this - unless you are doing custom protocol upgrades, as the expect/continue is automatically handled.

A handler could call sendResponseHeaders(101,0) and take over the streams but it isn't documented on how to do this.

Depending on the protocol, there may still be issues as well, as the connection timeouts could come into play.

I am still unclear as to why you needed this

I don't have a particular reason, saw the JDK had a bug and thought I might gain wisdom if I added a fix here.

and clearly it paid off

Also, if you’re inclined, I’m not against the PR to send the continue when the request is read. To me that makes more sense, so the handler has a chance to reject it. There is still the issue of the headers being sent instead of a set designed for the interim responses.

Seems to me that the ‘create interim response headers’ should be a callback on the exchange - but the JDK team has been very reluctant to make api changes. It’s like pulling teeth.

There is still the issue of the headers being sent instead of a set designed for the interim responses.

that we can change

It’s like pulling teeth.

Indeed

I don’t think we can change that without an api change by the JDK team. The exchange interface is defined by them.

That is, we don't send 100 with send response headers and just rely on the automatic send when reading

Yes, for 100 we would send no additional headers just like now - but we’d send them at the correct time allowing more efficient clients.