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.

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

Remove another side-effect in pool Slot machine #2228

Merged
merged 2 commits into from Oct 2, 2018

Conversation

Projects
None yet
4 participants
@jrudolph
Copy link
Member

jrudolph commented Sep 25, 2018

No description provided.

@jrudolph jrudolph requested a review from raboof Sep 25, 2018

@@ -375,9 +370,7 @@ private[client] object NewHostConnectionPool {
def pushRequest(request: HttpRequest): Unit = {
val newRequest =
request.entity match {
case _: HttpEntity.Strict
withSlot(_.onRequestEntityCompleted())

This comment has been minimized.

@jrudolph

jrudolph Sep 25, 2018

Member

onRequestEntityCompleted will not be called for Strict entities any more.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Sep 25, 2018

Test FAILed.

@jrudolph jrudolph force-pushed the jrudolph:jr/make-slot-state-machine-more-pure branch from 6a2a2b6 to a606691 Sep 25, 2018

@akka-ci akka-ci added validating and removed needs-attention labels Sep 25, 2018

@jrudolph jrudolph force-pushed the jrudolph:jr/make-slot-state-machine-more-pure branch from a606691 to 802ea00 Sep 25, 2018

@akka-ci akka-ci added needs-attention and removed validating labels Sep 25, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Sep 25, 2018

Test FAILed.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Sep 25, 2018

Test PASSed.

jrudolph added some commits Sep 25, 2018

=htc NewHostConnectionPool: remove weird pushRequestToConnectionAndTh…
…en HACK

The effect of the previous solution was that we had recursion of `updateState`
which we otherwise try to avoid really hard. It had a real effect on logs
where you could see the recursion in weird nesting of "Before" and "After"
log messages.

The new solution is to use another "virtual" state (+ event) to avoid the side-effecting
call in SlotState but instead add another state post processing clause.

@jrudolph jrudolph force-pushed the jrudolph:jr/make-slot-state-machine-more-pure branch from 802ea00 to 130360b Sep 25, 2018

@akka-ci

This comment has been minimized.

Copy link
Collaborator

akka-ci commented Sep 25, 2018

Test PASSed.

@johanandren
Copy link
Member

johanandren left a comment

LGTM

@jrudolph jrudolph merged commit ae43f59 into akka:master Oct 2, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@jrudolph jrudolph deleted the jrudolph:jr/make-slot-state-machine-more-pure branch Oct 2, 2018

@raboof
Copy link
Member

raboof left a comment

Love the simplification, the removal of the recursive state update in pushRequestToConnectionAndThen makes this much easier to follow. The inheritance makes it tricky to see if any cases were missed, though - perhaps could use some more cases in SlotStateSpec?

@@ -200,6 +196,13 @@ private[pool] object SlotState {
Unconnected
}
}
final case class PushingRequestToConnection(ongoingRequest: RequestContext) extends ConnectedState with BusyState {
override def waitingForEndOfRequestEntity: Boolean = ???

This comment has been minimized.

@raboof

raboof Oct 2, 2018

Member

This flag is used in BusyState to make sure that when the request is being failed while the response is already in flight, we correctly wait for the response to also finish before accepting new connections in this slot.

It seems like false would be a fine constant value in this case: when we haven't even dispatched the request yet, we don't need to wait for a response.

The fact that this does not fail any tests suggests we might want to cover some more corner cases there, though?

This comment has been minimized.

@jrudolph

jrudolph Oct 4, 2018

Member

I guess it works because this state is so short-lived that no events are ever dispatched to it. To improve the modelling we would have to separate actual states from requesting side-effects.

But I agree, setting this to false and commenting it would be the right way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment