#5685 - Add ServerResponse.reset() to support ErrorHandlers

[rbygrave]

• Fix for [nima][se] IllegalStateException("OutputStream already obtained"), when ErrorHandler uses ServerResponse.outputStream() #5685 - IllegalStateException("OutputStream already obtained"), when ErrorHandler uses ServerResponse.outputStream()
• Adds ServerResponse.reset() to reset response headers and buffers
• Only allowed to reset() when isSent is false

Fixes the test at: https://github.com/rbygrave/nima-error-handler/blob/main/src/test/java/org/example/MainTest.java

[oracle-contributor-agreement]

Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
The following contributors of this PR have not signed the OCA:

• PR author: @rbygrave
• robin.bygrave@gmail.com (@rbygrave)

To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application.

When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated.

If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public.

[rbygrave]

BTW: I have actually signed the OCA earlier this evening so not sure why that OCA Required is still showing up. Maybe it takes a while to sync that information.

[tomas-langer]

Seems to me the following could happen:

• user obtains outputStream
• user writes data to output stream
• (this causes all headers to be written to the output stream)
• code throws exception (without closing the stream)
• error handler calls reset
• error handler sends response

The state is now very bad - we have already sent status and headers and a part of entity, yet now error handler thinks it can write response status, headers and entity.
I think this would end in an inconsistent response data and a broken connection.

To work around this, we would need to keep track of the information if anything was written at all, and only allow reset if nothing was written (e.g. the response has not yet sent a single byte over the network).

Also this would need a test to validate such edge cases...

[rbygrave]

error handler calls reset

A call to reset() would not succeed if isSent is true. I can add a test for this case easily enough so I'll do that but there isn't an issue wrt not closing the OutputStream until it's actually sent content and isSent is then true. reset() should not be called (and will not succed) if isSent() is true.

user writes data to output stream (this causes all headers to be written to the output stream)

Just to say it does not actually send for the first call to os.write() [which is instead stored as firstBuffer] so if there has been only 1 call to os.write() it doesn't actually send anything yet. The second and subsequent calls to os.write() do though.

That is, the ServerResponse isSent() is false until ... a second call to os.write() or os.flush() or os.close() - until then it is isSent false and able to be reset (which means firstBuffer can be set back to null).

[rbygrave]

To work around this, we would need to keep track of the information if anything was written at all

Yes. As I see it that exists now as outputStream.totalBytesWritten()

, and only allow reset if nothing was written (e.g. the response has not yet sent a single byte over the network).

Yes. I get the test below to pass when isSent() is adjusted to be:

    @Override
    public boolean isSent() {
        return isSent ||  outputStream != null && outputStream.totalBytesWritten() > 0;
    }

The test as I have it is - https://github.com/rbygrave/nima-error-handler/blob/main/src/test/java/org/example/MainTest.java#L59-L67
... and this test passes with the change to isSent() and use of reset() at https://github.com/helidon-io/helidon/pull/5694/files#diff-b91d11cb7be7bc7310211e9669285fed86ad298e284c44f32fbcead79e9799ddR195-R197

Edit: Add the outputStream != null

[rbygrave]

Also this would need a test to validate such edge cases...

Ah, I found the integration tests in tests / integration / webserver / webserver ... which are like the tests I created (IntelliJ failed me for a bit there) - I can look to add tests there that are like the ones I created .

[rbygrave]

Needs a few adjustments here

[tomas-langer]

I think there should be a difference between isSent() and bytesWrittenOverNetwork()
The first (isSent()) is used as a very important check (see Filters.FilterChainImpl.proceed(), line 117) - we MUST complete the whole request/response interaction within the current virtual thread. So I really need to know that the response was fully sent, and no asynchronous handling will happen on the output after this point.
So modifying isSent() semantics will break some checks we have in place.

I can see two options how to solve this:

1. keep all check within the reset method, and return a boolean (e.g. success resetting is true, failure is false) In such a case, we do not need to change the API, other code can attempt to reset, and gracefully handle the case where it is just too late
2. add another public method such as dataSent() which would expose the information outside of response

I would prefer option 1 in this case, as it keeps the check internal to each specific implementation of response (HTTP/1 and HTTP/2) and does not expose information that is more of an implementation detail.

Please let me know if you want to continue on this feature yourself, or if you would prefer some other arrangement. If you want, you can contact me over our public slack channel to take care of the details

Thank you very much for your efforts on this!

[rbygrave]

I would prefer option 1

That makes good sense to me.

if you want to continue on this feature yourself

I can make that change for option 1. I'm not sure about the http2 tests and implementation and maybe someone needs to take that over.

I have no issue if someone wants to take over this PR or completely redo it. All good either way. I'll look to make the change to option 1 but all fine it someone wants to start again fresh on this.

[rbygrave]

I have pushed the change for "Option 1" to revert isSent() to original behavior and reset() now returns a boolean

let me know if you want to continue on this feature yourself

I've probably hit my knowledge limit here. In ErrorHandlers the code in this PR throws CloseConnectionException when it can't reset before handing over to a custom ErrorHandler which seems reasonable and gives the behavior I expect (incorrectly terminated chunked response) for Http1 but might not be correct especially for Http2 (there might be better desired behavior). With Http2 I have 2 tests in Http2ErrorHandlingWithOutputStreamTest that hang which I have commented out. I don't know how to move those forward.

I'm happy if someone wants to start afresh with a new PR or take this PR forward or suggest changes.

Cheers, Rob.

[tomas-langer]

Before I take over your code, could you kindly squash your commits into a single one (as I want to keep your commit in the history of Helidon)?
I will then check your PR and work on top of it to make sure it works both for HTTP/1 and HTTP/2.

Thanks!

[rbygrave]

squash your commits into a single one

Done.

check your PR and work on top of it to make sure it works both for HTTP/1 and HTTP/2

That would be awesome.

Cheers, Rob.

[tomas-langer]

@rbygrave I have rebased on latest main (some conflicting changes in ErrorHandlers). Could you please review my changes as well?
Thanks for your contribution!
(of course after I fix build issues)

[rbygrave]

Looks good. As a side comment, I prefer AssertJ :)

[tomas-langer]

Thank you for your contribution!