Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler FSM #214

Merged
merged 4 commits into from
Jul 13, 2018
Merged

Add SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler FSM #214

merged 4 commits into from
Jul 13, 2018

Conversation

mikkokar
Copy link
Contributor

Problem Description

Improve HttpPipelineHandler FSM to make it more predictable especially for some end to end tests.

There is a following scenario which sometimes causes intermittent e2e failures:

  1. Test client sends a request to Styx.
  2. Styx HTTP pipeline delivers a response in response observable onNext event.
  3. Styx sends the response.
  4. Client receives the response in full, and closes the TCP connection.
  5. Styx HttpPipelineHandler sees the channelInactive event prior to response observable onCompleted event.
  6. Styx treats this as a failed response (even if from client's point of view it was successful)
  7. Test case assertions fail.

Mostly this affects only those e2e tests whose assertions are based on observing changes in metrics. This has never caused any concern in production environments, where the incoming TCP connections tend to be pooled anyway.

The Fix

To remedy this situation, I have added a new SENDING_RESPON/SE_CLIENT_CLOSED state which allows HttpPipelineHandler to wait for onComplete from the response observable. If onComplete is seen the response is treated as success. Otherwise it is treated as failure.

@mikkokar mikkokar force-pushed the issue-199-request-cancellation-metric-per-origin branch from b1e8a97 to d270081 Compare July 12, 2018 14:05
@mikkokar mikkokar requested review from kvosper and dvlato July 12, 2018 14:06
dvlato
dvlato reviewed Jul 12, 2018
View reviewed changes
components/common/src/main/java/com/hotels/styx/common/StateMachine.java
@@ -203,5 +203,11 @@ public int hashCode() {
public StateMachine<S> build() {
return new StateMachine<>(initialState, stateEventHandlers, inappropriateEventHandler, stateChangeListener);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this method used? It doesn't show up if I look for 'debugTransitions' in this PR. Maybe we want to include this idea of (conditionally) logging state transitions instead of removing it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey David.

I would just leave this in. When debugging unit tests or e2e tests I can just add a .debugTransitions call to the state machine builder, and remove when ready.

Of course it could be integrated more nicely. 🤔

...onents/server/src/main/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandler.java Outdated
.transition(SENDING_RESPONSE_CLIENT_CLOSED, ResponseSentEvent.class, event -> onResponseSentAfterClientClosed(event.ctx))
.transition(SENDING_RESPONSE_CLIENT_CLOSED, ResponseWriteErrorEvent.class, event -> onResponseWriteError(event.ctx, event.cause))
.transition(SENDING_RESPONSE_CLIENT_CLOSED, ChannelInactiveEvent.class, event -> {
LOGGER.warn(warningMessage("Weird. This event/state was not supposed to happen."));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we handle this in the 'onInappropriateEvent' case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed we can. I will modify the code accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a change.

...ts/server/src/test/java/com/hotels/styx/server/netty/connectors/HttpPipelineHandlerTest.java Outdated
verify(statsCollector).onTerminate(request.id());
assertThat(adapter.state(), is(TERMINATED));
}
// @Test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we do something with this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have been looking at old diff?

@mikkokar mikkokar changed the title Add new SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler FSM Add SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler FSM Jul 13, 2018
@mikkokar mikkokar merged commit 16d01cf into ExpediaGroup:master Jul 13, 2018
mikkokar added a commit to mikkokar/styx that referenced this pull request Jul 13, 2018
@mikkokar
Add new SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler…
7705f6e 
… FSM (ExpediaGroup#214)

Handles TCP connection closures from connected clients more predictably.

(cherry picked from commit 16d01cf)
mikkokar added a commit that referenced this pull request Jul 13, 2018
@mikkokar
Add new SENDING_RESPONSE_CLIENT_CLOSED state into HttpPipelineHandler…
e27c452 
… FSM (#214) (#216)

Handles TCP connection closures from connected clients more predictably.

(cherry picked from commit 16d01cf)
@mikkokar mikkokar deleted the issue-199-request-cancellation-metric-per-origin branch April 1, 2019 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvlato dvlato dvlato left review comments

@kvosper kvosper kvosper approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mikkokar @kvosper @dvlato