Remove RxJava - Part 4 #560

OwenLindsell · 2019-12-10T08:51:03Z

Request body now follows reactive streams back pressure protocol.

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

mikkokar

Hi @OwenLindsell This code is broken. But not because of your changes (apart from my another comment, see below).

This class fails to correctly follow the reactive back pressure protocol. It merely triggers a channel read every on each back pressure request. But event count tracking is completely missing.

As a background info: it was designed that way: to allow only one request at the time. But I think the original implementation was broken. But we never had a need to fix this.

I suggest now is the time to roll in the FlowControllingHttpContentProducer from styx client to rectify this issue.

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

…th-reactor-part4

components/common/src/main/java/com/hotels/styx/common/content/BufferSubscriber.java

mikkokar · 2020-01-15T11:48:14Z

components/common/src/main/java/com/hotels/styx/common/content/ContentPublisher.java

+/**
+ * A publisher to wrap the FlowControllingHttpContentProducer FSM and perform subscription operations via a Netty EventLoop.
+ */
+public final class ContentPublisher implements Publisher<Buffer> {


Perhaps we should rename this to ByteStreamPublisher? Because it is the ByteStream publisher for which this class produces content. That might make it easier for other devs make the connection.

That couples it deeply to the ByteStream class, which it knows nothing about. It only knows about the FlowControllingHttpContentProducer and even that doesn't know about ByteStream. However, I get your point, the name is not very meaningful, so I have renamed it to FlowControllingPublisher. Is that more helpful?

Hi. It was just a suggestion. I'm not too opinionated about this. But between FlowControllingPublisher and ContentPublisher, I would prefer the original because it is shorter and simpler :-)

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

mikkokar · 2020-01-15T14:59:36Z

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

            }
        } catch (BadRequestException ex) {
            throw ex;
        } catch (Exception ex) {
-            throw new BadRequestException(ex.getMessage() + " in " + httpObject, ex);
+            throw new BadRequestException(ex.getMessage() + " in " + msg, ex);
        }


Consider what happens when the try block throws. For example due to headers decoding failure. This code would throw, and the resulting LiveHttpRequest is never propagated up to the HTTP pipeline handler. But all the subsequent content chunks would still get queued in the flow controller.

This would end up leaking (direct) memory.

The easiest way to handle this is to pass the exception to the FSM:

} catch (NNNException ex) { getContentProducer().channelException(ex); throw ex; }

This will move the FSM to a TERMINATED state, where all subsequent content chunks are automatically down referenced.

We need to have a test case for this scenario, if possible.

I considered that. I think the exception will be handled by Netty, which will pass it onto the exceptionCaught method, which calls the FSM and then propagates it to the HttpPipelineHandler. Or have I not understood it correctly?

Hi yes. The exception will propagate. And ideally the pipeline handler responds by closing the connection. It is still better to be defensive, and move the FSM to TERMINATED. That guarantees that any extra pending content chunk events will be correctly dealt with. Also the FSM state would be consistent with the error, in case someone ever needs to debug it.

The exceptionCaught method makes the same call to the FSM, so it will move it to terminated.

mikkokar · 2020-01-15T15:06:51Z

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

+        ctx.channel().eventLoop().schedule(
+                () -> producer.ifPresent(FlowControllingHttpContentProducer::tearDownResources),
+                TEARDOWN_DELAY,
+                MILLISECONDS);


Now looking back retrospectively, it seems bit funny that we tear down resources by generating a ResponseTimeoutException with "channelClosed" reason phrase. Let's think about improving this in the near future.

By the same token, I now realise that the scheduleTeardown trigger should be different for uplink traffic.

The FSM was developed for downlink traffic. In downlink direction we cannot have two back-to-back responses without subscriber fully consuming the response body. Therefore in downlink we can use channel idleness to drive the scheduleTeardown event.

The FSM must go to the COMPLETED state before the channel can be reused for the next request. Because origin cannot respond anything before we send the another request, the cannel (TCP connection) would remain idle if the content subscriber never consumes the content.

But for uplink, I believe, this assumption is not valid. It is possible for an origin to respond without consuming the request body at all. This response would allows a remote a sender to send a new request even if Styx never consumed the original request body. Therefore we cannot rely on channel idleness to clean up resources.

We need to think of something else that is guaranteed to trigger a clean up.

…th-reactor-part4 # Conflicts: # components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java # components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

dvlato · 2020-01-20T15:52:41Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

@@ -70,6 +83,9 @@

    private volatile Subscriber<? super ByteBuf> contentSubscriber;

+    private final HashedWheelTimer timer = new HashedWheelTimer();


From HashedWheelTimer:

Do not create many instances. HashedWheelTimer creates a new thread whenever it is instantiated and started. Therefore, you should make sure to create only one instance and share it across your application. One of the common mistakes, that makes your application unresponsive, is to create a new instance for every connection.

However, it seems we are creating a Timer for every http request we receive.

changed to static

dvlato

Please, check my comment about HashedWheelTimer

dvlato · 2020-01-20T15:57:47Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

 import io.netty.util.ReferenceCountUtil;
+import io.netty.util.Timeout;
+import io.netty.util.Timer;


Import seems unnecessary.

dvlato · 2020-01-20T15:57:57Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

 import org.reactivestreams.Subscriber;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

 import java.util.Queue;
 import java.util.concurrent.ConcurrentLinkedDeque;
+import java.util.concurrent.TimeUnit;


Import seems unnecessary

dvlato · 2020-01-20T15:59:53Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java


+        TimerTask timerTask = timeout -> {


we don't need curly braces here

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

dvlato · 2020-01-20T16:09:29Z

components/client/src/test/unit/resources/logback.xml

@@ -16,6 +16,5 @@
  <logger name="com.hotels.styx.http-messages.inbound" level="INFO"/>
  <logger name="com.hotels.styx.http-messages.outbound" level="INFO"/>
  <logger name="com.hotels.styx.client.OriginRestrictionLoadBalancingStrategy" level="INFO"/>
-  <logger name="com.hotels.styx.client.netty.connectionpool.FlowControllingHttpContentProducer" level="INFO"/>


should we remove this or just rename with the appropriate package name?

remove it as the class has moved to a different module

dvlato · 2020-01-20T16:14:47Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

    private static final Logger LOGGER = LoggerFactory.getLogger(FlowControllingHttpContentProducer.class);
    private static final int MAX_DEPTH = 1;

-    private final StateMachine<ProducerState> stateMachine;


This can be kept final

yes, not sure why I removed final

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

…th-reactor-part4

David comments have been addressed and David left the company

OwenLindsell

made changes as requested :)

OwenLindsell · 2020-01-27T11:12:41Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

    private static final Logger LOGGER = LoggerFactory.getLogger(FlowControllingHttpContentProducer.class);
    private static final int MAX_DEPTH = 1;

-    private final StateMachine<ProducerState> stateMachine;


yes, not sure why I removed final

OwenLindsell · 2020-01-27T11:13:29Z

.../common/src/main/java/com/hotels/styx/common/content/FlowControllingHttpContentProducer.java

@@ -70,6 +83,9 @@

    private volatile Subscriber<? super ByteBuf> contentSubscriber;

+    private final HashedWheelTimer timer = new HashedWheelTimer();


changed to static

OwenLindsell

fixed

OwenLindsell

fixed

mikkokar · 2020-04-07T11:10:49Z

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

-            getContentProducer(ctx).tearDownResources("idle timeout");
+            FlowControllingHttpContentProducer producer = getContentProducer(ctx);
+            getContentProducer(ctx).tearDownResources(
+                new ResponseTimeoutException(


A strictly correct behaviour is to use a ResponseTimeoutException whenever the HTTP headers have yet not been received, and a ContentTimeoutException otherwise. We could use a presence of producer to determine this.

But this retains the existing behaviour, and therefore we can do this as a separate increment. Could you raise a GH issue for this please?

mikkokar · 2020-04-07T11:22:42Z

HI @OwenLindsell there are check style related changes to the pom.xml files. They look bit unrelated. Could you add a quick comment to this PR to shed some light on them?

Just a quick note will serve as a record for the future maintainers. This would be really helpful.

components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java

OwenLindsell · 2020-04-07T11:37:03Z

HI @OwenLindsell there are check style related changes to the pom.xml files. They look bit unrelated. Could you add a quick comment to this PR to shed some light on them?

Just a quick note will serve as a record for the future maintainers. This would be really helpful.

pom changes were reverted yesterday

…th-reactor-part4

… an InactiveSubscriberException or a ResponseTimeoutException. This is because both timeouts are set by the same config - idleResponseTimeout. In the future we should implement these to be separately configurable.

…th-reactor-part4

remove rx java

2bc6b5b

dvlato reviewed Dec 10, 2019

View reviewed changes

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java Outdated Show resolved Hide resolved

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java Show resolved Hide resolved

dvlato approved these changes Dec 10, 2019

View reviewed changes

simplify

5817ae5

mikkokar suggested changes Dec 12, 2019

View reviewed changes

...nents/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java Outdated Show resolved Hide resolved

OwenLindsell added 3 commits December 17, 2019 15:13

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

c987f01

…th-reactor-part4

replace fsm in NettyToStyxRequestDecoder

0e68ba8

replace fsm in NettyToStyxRequestDecoder

560f49c

OwenLindsell requested a review from mikkokar January 13, 2020 14:53

OwenLindsell added 3 commits January 13, 2020 15:07

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

52a5909

…th-reactor-part4

replace fsm in NettyToStyxRequestDecoder

83e40ea

replace fsm in NettyToStyxRequestDecoder

d75f50b

mikkokar reviewed Jan 15, 2020

View reviewed changes

mikkokar suggested changes Jan 15, 2020

View reviewed changes

OwenLindsell added 3 commits January 15, 2020 15:52

remove rxJava

23b16e0

Added WheelHashedTimer to timeout inactive subscribers

4ba97ec

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

7219df7

…th-reactor-part4 # Conflicts: # components/proxy/src/main/java/com/hotels/styx/ProxyConnectorFactory.java # components/server/src/main/java/com/hotels/styx/server/netty/codec/NettyToStyxRequestDecoder.java

dvlato reviewed Jan 20, 2020

View reviewed changes

dvlato previously requested changes Jan 20, 2020

View reviewed changes

dvlato reviewed Jan 20, 2020

View reviewed changes

OwenLindsell added 7 commits January 22, 2020 09:39

made timer static

f5c27b2

removed unused imports

7037875

added tests for inactive subscriber functionality

09ba6c8

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

3378b60

…th-reactor-part4

added tests for inactive subscriber functionality

4ee2c0c

empty commit

26df1f2

empty

940d4d3

moved last active to timer class

54e5148

OwenLindsell requested a review from mikkokar April 6, 2020 12:53

OwenLindsell added 9 commits April 6, 2020 14:04

revert change to checkstyle

1d68912

revert change to checkstyle

0d9f206

removed origin in call to FlowControllingHttpContentProducer

7fce7c1

removed redundant call

2fa01e5

restored accidentally modified test

3c49a9f

renamed class and field

83c921b

removed unused class

172d4d2

restored accidentally deleted code

8ec97e0

removed tab that caused test to fail

386535c

OwenLindsell commented Apr 7, 2020

View reviewed changes

mikkokar reviewed Apr 7, 2020

View reviewed changes

components/client/src/test/unit/java/com/hotels/styx/client/StyxHttpClientTest.java Show resolved Hide resolved

removed temp commenting

928a624

mikkokar approved these changes Apr 7, 2020

View reviewed changes

OwenLindsell added 7 commits April 7, 2020 14:20

empty commit

36e5f60

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

cfc7329

…th-reactor-part4

empty commit

edfaccc

removed unused import

1e5fea2

Merge remote-tracking branch 'upstream/master' into replace-rxjava-wi…

cb37310

…th-reactor-part4

restored constructor that was deleted but is still required

5e318bf

OwenLindsell merged commit cd2ae43 into ExpediaGroup:master Apr 8, 2020

OwenLindsell deleted the replace-rxjava-with-reactor-part4 branch April 9, 2020 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove RxJava - Part 4 #560

Remove RxJava - Part 4 #560

OwenLindsell commented Dec 10, 2019 •

edited by mikkokar

Loading

mikkokar left a comment

mikkokar Jan 15, 2020

OwenLindsell Jan 15, 2020

mikkokar Jan 15, 2020

mikkokar Jan 15, 2020

OwenLindsell Jan 15, 2020

mikkokar Jan 15, 2020

OwenLindsell Jan 15, 2020

mikkokar Jan 15, 2020

mikkokar Jan 15, 2020

dvlato Jan 20, 2020

OwenLindsell Jan 27, 2020

dvlato left a comment

dvlato Jan 20, 2020

OwenLindsell Jan 22, 2020

dvlato Jan 20, 2020

OwenLindsell Jan 22, 2020

dvlato Jan 20, 2020

OwenLindsell Jan 27, 2020

dvlato Jan 20, 2020

OwenLindsell Jan 27, 2020

dvlato Jan 20, 2020

OwenLindsell Jan 27, 2020

OwenLindsell left a comment

OwenLindsell Jan 27, 2020

OwenLindsell Jan 27, 2020

OwenLindsell left a comment

OwenLindsell left a comment

mikkokar Apr 7, 2020

OwenLindsell Apr 7, 2020

mikkokar commented Apr 7, 2020

OwenLindsell commented Apr 7, 2020

		@@ -70,6 +83,9 @@

		private volatile Subscriber<? super ByteBuf> contentSubscriber;

		private final HashedWheelTimer timer = new HashedWheelTimer();

Remove RxJava - Part 4 #560

Remove RxJava - Part 4 #560

Conversation

OwenLindsell commented Dec 10, 2019 • edited by mikkokar Loading

mikkokar 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

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 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenLindsell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenLindsell left a comment

Choose a reason for hiding this comment

OwenLindsell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkokar commented Apr 7, 2020

OwenLindsell commented Apr 7, 2020

OwenLindsell commented Dec 10, 2019 •

edited by mikkokar

Loading