HttpInput refactoring #4556

lorban · 2020-02-07T10:56:41Z

This is a work in progress, the PR is only about exercising the code against the full test suite under Jenkins. As a reminder, this work was triggered by #4318.

Old status:

HTTP 1 and FCGI have been reimplemented and nearly pass the full test suite (Milestone 1)
HTTP 2 has been reimplemented and nearly passes the full test suite (Milestone 2)
All HttpInput* implementations pass the full test suite (Milestone 3)
HTTP 1, 2 and FCGI implementations share 90% of their code, don't queue and pass the full test suite (Milestone 4)
addContent() doesn't return a boolean that is used for scheduling. (Milestone 5)
HTTP 1, 2 and FCGI use the exact same implementation, all differences having been pushed to HttpChannel (Milestone 6).

Current status:

Got rid of addContent() by making produceContent() return Content instead.
Make EOF and errors be special content.
Transition to a much simplified FSM by using the needContent() / produceContent() model.
Implement blocking on top of async, this way there is only one FSM.

Next steps:

Code review.

gregw

just some thoughts from a light review in passing...

jetty-server/src/main/java/org/eclipse/jetty/server/AbstractLockedHttpInput.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInputOverHTTP.java

jetty-server/src/main/java/org/eclipse/jetty/server/AbstractHttpInput.java

jetty-server/src/main/java/org/eclipse/jetty/server/AbstractLockedHttpInput.java

gregw · 2020-03-12T12:52:03Z

@lorban To help review, it would be good to get updates of:

gregw

review in progress...

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java

sbordet · 2020-04-13T15:28:17Z

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

-            HttpInput.Content content = takeContent(endStreamResult);
-            if (content != null)
-                return content;
+            getStream().demand(0);


I'm not particularly fond of this - I'm sure it can be done otherwise.
The demand mechanism is well established now (even present in the JDK) so I would not change its semantic.

The reason why I introduced this mechanism is to make HTTP 1 and 2 more similar and avoid triggering data demands until we reach a safe spot in the state machine, much like HTTP 1 makes the distinction between parseAndFill and setting fillInterested.

This drastically helps controlling the paths that could lead to race conditions as the latter can only happen wherever we decide they can happen instead of virtually everywhere, making debugging a lot more manageable.

I tried really hard without touching this mechanism and the fact that you can't read data without triggering an async call that adds content makes the code, IMHO, virtually unmaintainable.

We need to discuss this more, as we have certainly figured out more complex cases, and I don't feel this one is impossible to solve without breaking the demand semantic - which would potentially introduce a much larger problem.

Yes, I agree.

How about just adding an available() method to the interface. All that is necessary is something that can be used to query to decide if we should continue iterating on content or exit and let HttpChannel.unhandle do the demand(1) to setup to receive more content. So long as we don't have content arriving between those two events, then things become simpler.

I like the idea of adding an available() method a lot: it's a cleaner interface, trivial to implement and provides the necessary semantic.

gregw · 2020-06-13T08:44:57Z

Just a note on status of my much interrupted redux review of this PR.
I have a branch that has a few non consequential changes (cleanups, renames etc.)... will push that soonish

I am working through the interaction diagrams of the main flows for h1|h2 x blocking| async. I have still some more work to do there to get a good overall picture before deciding how I think it should change.... but I'll note down my current thinking here & now, just incase I get interrupted again and forget.

The issue for me is how in some scenarios the callbacks are doing the content processing, while in others they don't. Thecrux for my is h2 callbacks with call addContent, which in turn calls available (to see if that raw content actually produced real content) and then decides if handle should be called or not. This is done outside of the normal handle cycle, so it is racey and needs protection etc.

So I think we can greatly improve this by not having addContent call available. The h2 callbacks will still add the raw content, but it will not be immediately processed, instead they will return true if a wakeup is needed and then we will call HttpChannel.handle, which should be in the REGISTER (or may PRODUCING???) state and it can call available() (or something similar) to process raw content to see if there is real content and then decide to call onReadPossible or not. The processing is then done in the better protected handling loop, the same as for h1..... at least that is my bubble of an idea.. Lot's more detailed analysis still needed for the exact state transitions.

gregw · 2020-08-18T12:02:17Z

@lorban don't forget the interaction diagrams! I know you are going step by step towards your destination.... but we don't want you to get there only to discover that the usual malcontents didn't want to go there in the first place!

Please have a go of updating them to what you think they should be and check them in.

gregw

I couple of little things while I get on with deeper review...

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java

jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2StreamEndPoint.java

...y-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/ContentDemander_state.puml

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

gregw · 2020-10-14T21:22:50Z

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

+            if (content != null)
+                return content;
+
+            _semaphore.drainPermits();


I really REALLY don't like having drainPermits. This should not be necessary

gregw · 2020-10-14T21:47:52Z

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

            boolean handleRequest = onRequestComplete();
            handle |= handleContent | handleRequest;
        }

+        boolean woken = getRequest().getHttpInput().wakeup();


I think this is wrong because it means a spurious wakeup can occur even if needContent has not been called.

A call to produceContent can do demand, so onData may be called at any time afterwards... so it is wrong for it to wakeup HttpInput if needContent has not been called.

This class needs to know if needContent has been called and only do a wakeup if it has.

The more I think about this, the more I'm convinced we need a dedicated onContentAvailable callback that is only called if needContent has been called.

We could almost pass a Callback in needContent to make the contract clear!

jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java

jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java

jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java

jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java

sbordet · 2020-10-14T20:20:04Z

jetty-server/src/main/java/org/eclipse/jetty/server/ContentProducer.java

+    boolean isError();
+
+    /**
+     * This call may block.


Why this call may block? Isn't it wrong that there is a blocking method if it can be implemented by AsyncContentProducer?

Because of poor javadoc. What I meant is that some implementations may decide to make nextContent() a blocking call, some others may not. This is exactly what BlockingContentProducer and AsyncContentProducer do but I'd like the javadoc to avoid speaking about specific implementations.

sbordet · 2020-10-14T20:32:33Z

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java

+    // events like timeout: we get notified and either schedule onError or release the
+    // blocking semaphore.
+    private HttpInput.Content _content;
+    private HttpInput.Content _specialContent;


It's not true that _specialContent is only accessed by one thread. It is set by the scheduler thread during idle timeouts.

My original analysis was that the scheduler thread only ever writes to _specialContent, then either notifies the blocked thread with the semaphore (which gives us a happens-before) or reschedules a thread that reads it. A job submission to a standard ExecutorService creates a happens-before relationship that would make this safe but I may need to check if the form of scheduling we use in EWYK does too. I suspect it does, but I'll review this in more details.

Looks safe to me, but I could mark _specialContent as volatile nevertheless. This would make the code a bit less brittle in wrt potential future changes.

why do we need both _content and _specialContent. We only have a single content in H2 and I think similar logic could be applied here.

I got rid of _specialContent.

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

gregw · 2020-10-15T09:08:59Z

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

+
+            _semaphore.drainPermits();
+            _httpChannel.getState().onReadUnready();
+            if (_httpChannel.needContent())


It needs to be PRECISELY that only when needContent returns false will there be some callback that will release the semaphore. So we need to only ever acquire the semaphore in that case. We then don't need drain permits. I'm still thinking needContent(Callback) is the way to go.

sbordet · 2020-10-15T09:02:03Z

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

+    @Override
+    public boolean wakeup()
+    {
+        // Calling _asyncContentProducer.wakeup() breaks AsyncIOServletTest.testAsyncWriteThrowsError H2C.


Also, feels weird that we are not forwarding to _asyncContextProducer -- if really not needed, it deserves a comment.

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java

jetty-server/src/test/java/org/eclipse/jetty/server/BlockingContentProducerTest.java

...st-http-client-transport/src/test/java/org/eclipse/jetty/http/client/AsyncIOServletTest.java

...est-http-client-transport/src/test/java/org/eclipse/jetty/http/client/TransportScenario.java

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java

jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java

sbordet

LGTM, apart few niceties and a little more javadocs.

jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

sbordet · 2020-11-03T11:50:37Z

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

+                else if (c == null)
+                {
+                    if (!content.isSpecial())
+                        throw new AssertionError("Non special content without demand");


I insist that we cannot throw, even if it's a bug.
Throwing will cause the server to hang and require a restart, while failing the callback will fail the current request and allow for future requests.

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java

jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

…CGI (Milestone 3) Signed-off-by: Ludovic Orban <lorban@bitronix.be>

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

…to HttpChannel* impls (Milestone 5) Signed-off-by: Ludovic Orban <lorban@bitronix.be>

…tead. Make EOF and errors be special content. Transition to a much simplified FSM by using the needContent() / produceContent() model. Implement blocking on top of async, this way there is only one FSM. (Milestone 6) Signed-off-by: Ludovic Orban <lorban@bitronix.be>

gregw requested changes Feb 25, 2020

View reviewed changes

gregw reviewed Mar 12, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java Outdated Show resolved Hide resolved

gregw reviewed Mar 12, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java Outdated Show resolved Hide resolved

gregw reviewed Mar 12, 2020

View reviewed changes

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java Outdated Show resolved Hide resolved

gregw reviewed Mar 12, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java Outdated Show resolved Hide resolved

gregw requested changes Mar 27, 2020

View reviewed changes

sbordet reviewed Apr 13, 2020

View reviewed changes

lorban marked this pull request as ready for review October 7, 2020 07:00

lorban requested review from gregw and sbordet October 7, 2020 07:00

gregw requested changes Oct 12, 2020

View reviewed changes

gregw reviewed Oct 12, 2020

View reviewed changes

jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java Show resolved Hide resolved

gregw reviewed Oct 12, 2020

View reviewed changes

jetty-http2/http2-common/src/main/java/org/eclipse/jetty/http2/HTTP2StreamEndPoint.java Outdated Show resolved Hide resolved

gregw reviewed Oct 12, 2020

View reviewed changes

...y-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/ContentDemander_state.puml Show resolved Hide resolved

gregw reviewed Oct 12, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java Outdated Show resolved Hide resolved

gregw reviewed Oct 12, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelOverHttp.java Outdated Show resolved Hide resolved

gregw requested changes Oct 14, 2020

View reviewed changes

gregw reviewed Oct 14, 2020

View reviewed changes

sbordet reviewed Oct 15, 2020

View reviewed changes

gregw reviewed Oct 15, 2020

View reviewed changes

sbordet requested changes Oct 15, 2020

View reviewed changes

lorban requested review from gregw and sbordet October 26, 2020 13:39

gregw reviewed Oct 26, 2020

View reviewed changes

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java Outdated Show resolved Hide resolved

gregw reviewed Oct 26, 2020

View reviewed changes

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java Outdated Show resolved Hide resolved

gregw reviewed Oct 26, 2020

View reviewed changes

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java Outdated Show resolved Hide resolved

gregw reviewed Oct 26, 2020

View reviewed changes

jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java Outdated Show resolved Hide resolved

gregw reviewed Oct 26, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/BlockingContentProducer.java Show resolved Hide resolved

lorban requested a review from gregw October 28, 2020 09:36

gregw requested changes Nov 3, 2020

View reviewed changes

jetty-server/src/main/java/org/eclipse/jetty/server/AsyncContentProducer.java Show resolved Hide resolved

sbordet approved these changes Nov 3, 2020

View reviewed changes

gregw approved these changes Nov 3, 2020

View reviewed changes

lorban added 6 commits November 3, 2020 17:30

HTTP and FCGI pass all tests (Milestone 1)

5a24c8d

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

http2 impl (Milestone 2)

946f334

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

FCGI queue and pull model, merge all common code between H1, H2 and F…

982ddc0

…CGI (Milestone 3) Signed-off-by: Ludovic Orban <lorban@bitronix.be>

addContent now returns void (Milestone 4)

d0f04a8

Signed-off-by: Ludovic Orban <lorban@bitronix.be>

Merge all HttpInput* impls into a single class, push all differences …

a4258ec

…to HttpChannel* impls (Milestone 5) Signed-off-by: Ludovic Orban <lorban@bitronix.be>

sbordet merged commit ebea687 into jetty:jetty-10.0.x Nov 3, 2020

lorban deleted the http-input branch November 4, 2020 09:36

sbordet mentioned this pull request Mar 24, 2021

Review HTTP/2 DATA frame queueing #4318

Closed

This was referenced Dec 14, 2021

Interceptors don't get destroyed in HttpInput #7280

Closed

EOFs are not passed to interceptors any more - shouldn't they? #7281

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpInput refactoring #4556

HttpInput refactoring #4556

lorban commented Feb 7, 2020 •

edited

Loading

gregw left a comment

gregw commented Mar 12, 2020

gregw left a comment

sbordet Apr 13, 2020 •

edited

Loading

lorban Apr 13, 2020

sbordet Apr 13, 2020

lorban Apr 13, 2020

gregw Apr 14, 2020

lorban Apr 14, 2020

gregw commented Jun 13, 2020

gregw commented Aug 18, 2020

gregw left a comment

gregw Oct 14, 2020

gregw Oct 14, 2020

gregw Oct 15, 2020

gregw Oct 15, 2020

sbordet Oct 14, 2020

lorban Oct 15, 2020

sbordet Oct 14, 2020

lorban Oct 15, 2020 •

edited

Loading

lorban Oct 20, 2020

gregw Oct 26, 2020

lorban Oct 28, 2020

gregw Oct 15, 2020

sbordet Oct 15, 2020

sbordet left a comment

sbordet Nov 3, 2020

HttpInput refactoring #4556

HttpInput refactoring #4556

Conversation

lorban commented Feb 7, 2020 • edited Loading

gregw left a comment

Choose a reason for hiding this comment

gregw commented Mar 12, 2020

gregw left a comment

Choose a reason for hiding this comment

sbordet Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregw commented Jun 13, 2020

gregw commented Aug 18, 2020

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbordet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorban commented Feb 7, 2020 •

edited

Loading

sbordet Apr 13, 2020 •

edited

Loading

lorban Oct 15, 2020 •

edited

Loading