Fixes #118 Improve FlowControllingHttpContentProducer state machine. #120

dvlato · 2018-03-27T08:44:22Z

New event, DelayedTearDownEvent, created to replace IdleStateEvent.
Currently there's no handling of idle events in the FSM in FlowControllingHttpContentProducer, so the logic has been kept untouched.

…te machine. New event DelayedTearDownEvent created to replace IdleStateEvent.

mikkokar · 2018-03-27T10:15:59Z

...src/main/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagator.java

-                                producer.receivedChunks(),
-                                producer.emittedBytes(),
-                                producer.emittedChunks()))),
+                () -> contentProducer.ifPresent(producer -> producer.tearDownResources(cause)),


The tearDownResources event is scheduled in a controlled fashion. That is, the tearDownResources is always scheduled after a short time to ensure state machine is orderly brought down. Therefore cause doesn't have to be passed in.

I would say that if we have to publish an error, it will be more informative if it is the same error that caused the "channelInactive" -so they can be easily linked when reading the logs-, than if we make up an Exception as in your example in the comment below (TimedOutExceptionWhatever).

I have nonetheless implemented the change as per your suggestion keeping the ResponseTimeoutException as previously.

mikkokar · 2018-03-27T10:20:45Z

...ain/java/com/hotels/styx/client/netty/connectionpool/FlowControllingHttpContentProducer.java

                .transition(BUFFERING_COMPLETED, ContentSubscribedEvent.class, this::contentSubscribedEventWhileBufferingCompleted)
                .transition(BUFFERING_COMPLETED, ContentEndEvent.class, s -> BUFFERING_COMPLETED)

                .transition(STREAMING, RxBackpressureRequestEvent.class, this::rxBackpressureRequestEventWhileStreaming)
                .transition(STREAMING, ContentChunkEvent.class, this::contentChunkEventWhileStreaming)
                .transition(STREAMING, ChannelInactiveEvent.class, e -> emitErrorAndTerminate(e.cause()))
                .transition(STREAMING, ChannelExceptionEvent.class, e -> emitErrorAndTerminate(e.cause()))
-                .transition(STREAMING, ChannelIdleEvent.class, e -> emitErrorAndTerminate(new ResponseTimeoutException(origin)))
+                .transition(STREAMING, DelayedTearDownEvent.class, e -> emitErrorAndTerminate(e.cause()))


I would use a different method for the state transition handler. For example:

+ .transition(STREAMING, DelayedTearDownEvent.class, e -> tearDown())

And then in the tearDown method:

private void tearDown() { emitErrorAndTerminate(new TimedOutExceptionWhatever()); // Choose an appropriate exception here. }

So I would have a dedicated method specifically for the tearDown transition. This exposes the state transition logic more explicitly.

That sounds like an improvement, but in your example we are emitting an error, I assume, to signal an invalid combination of state+event. Doesn't it make more sense to use a new 'tearDown' method for the cases in which we are calling 'releaseAndTerminate' ?

…te machine. New event DelayedTearDownEvent created to replace IdleStateEvent.

… necessary states: BUFFERING_COMPLETED EMITTING_BUFFERED_CONTENT

mikkokar · 2018-04-05T14:37:38Z

...ain/java/com/hotels/styx/client/netty/connectionpool/FlowControllingHttpContentProducer.java

@@ -53,6 +53,7 @@
    private final Runnable askForMore;
    private final Runnable onCompleteAction;
    private final Consumer<Throwable> onTerminateAction;
+    private Runnable delayedTearDownAction;


This can be final.

mikkokar · 2018-04-05T14:46:14Z

...ain/java/com/hotels/styx/client/netty/connectionpool/FlowControllingHttpContentProducer.java

                .transition(BUFFERING_COMPLETED, ChannelExceptionEvent.class, s -> BUFFERING_COMPLETED)
-                .transition(BUFFERING_COMPLETED, ChannelIdleEvent.class, this::releaseAndTerminate)
+                .transition(BUFFERING_COMPLETED, DelayedTearDownEvent.class, this::tearDown)


I am thinking we should call this::releaseAndTerminate directly since the this::tearDown method is identical.

mikkokar · 2018-04-05T14:48:43Z

...ain/java/com/hotels/styx/client/netty/connectionpool/FlowControllingHttpContentProducer.java

                .transition(EMITTING_BUFFERED_CONTENT, ChannelExceptionEvent.class, s -> EMITTING_BUFFERED_CONTENT)
-                .transition(EMITTING_BUFFERED_CONTENT, ChannelIdleEvent.class, e -> emitErrorAndTerminate(e.cause()))
+                .transition(EMITTING_BUFFERED_CONTENT, DelayedTearDownEvent.class, this::tearDownWithError)


Perhaps it is better to use emitErrorAndTerminate instead of tearDownWithError because they are identical.

mikkokar · 2018-04-05T14:50:23Z

...ain/java/com/hotels/styx/client/netty/connectionpool/FlowControllingHttpContentProducer.java

+
+    private ProducerState tearDown(CausalEvent event) {
+        return releaseAndTerminate(event);
+    }


As mentioned in the previous comments let's get rid of tearDown and tearDownWithError methods.

mikkokar · 2018-04-05T15:01:52Z

...src/main/java/com/hotels/styx/client/netty/connectionpool/NettyToStyxResponsePropagator.java

-                                producer.receivedBytes(),
-                                producer.receivedChunks(),
-                                producer.emittedBytes(),
-                                producer.emittedChunks()))),


Please ensure these stats are passed in to the ResponseTimeoutException.
Even better please add a test that verifies that the counters are present in the exception when it is torn down.

Or instead log a warning using FlowControllingHttpContentProducer.warningMessage. This is the preferred option IMO.

The log messages can be tested with LoggingTestSupport. Please look for examples of usage in the codebase. It is quite easy to use.

Alternatively raise a separate GitHub enhancement issue.

TearDown methods removed Exception in the teardownevent contains bytecount.

Fixes ExpediaGroup#118 Improve FlowControllingHttpContentProducer sta…

f6b7171

…te machine. New event DelayedTearDownEvent created to replace IdleStateEvent.

mikkokar suggested changes Mar 27, 2018

View reviewed changes

a-dlatorre added 3 commits March 27, 2018 17:19

Fixes ExpediaGroup#118 Improve FlowControllingHttpContentProducer sta…

e1827c0

…te machine. New event DelayedTearDownEvent created to replace IdleStateEvent.

Tear down event will be scheduled from the state machine only for the…

a7d9bac

… necessary states: BUFFERING_COMPLETED EMITTING_BUFFERED_CONTENT

Tear down event will be scheduled from the state machine only for the…

562ad57

… necessary states: BUFFERING_COMPLETED EMITTING_BUFFERED_CONTENT

mikkokar suggested changes Apr 5, 2018

View reviewed changes

Final modifier added

536da87

TearDown methods removed Exception in the teardownevent contains bytecount.

mikkokar approved these changes Apr 9, 2018

View reviewed changes

mikkokar merged commit 153ec50 into ExpediaGroup:master Apr 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #118 Improve FlowControllingHttpContentProducer state machine. #120

Fixes #118 Improve FlowControllingHttpContentProducer state machine. #120

dvlato commented Mar 27, 2018

mikkokar Mar 27, 2018

dvlato Mar 27, 2018 •

edited

Loading

mikkokar Mar 27, 2018

dvlato Mar 27, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

mikkokar Apr 5, 2018

Fixes #118 Improve FlowControllingHttpContentProducer state machine. #120

Fixes #118 Improve FlowControllingHttpContentProducer state machine. #120

Conversation

dvlato commented Mar 27, 2018

Choose a reason for hiding this comment

dvlato Mar 27, 2018 • 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

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

dvlato Mar 27, 2018 •

edited

Loading