=con #17670 Fix potential ReceivePipeline MatchError #17678

jeremystone · 2015-06-06T20:31:39Z

Patch that should hopefully fix #17670

akka-ci · 2015-06-06T20:36:02Z

Can one of the repo owners verify this patch?

ktoso · 2015-06-15T15:38:19Z

akka-contrib/src/main/scala/akka/contrib/pattern/ReceivePipeline.scala

        aroundCache = Some((receive, zipped))
        zipped
    }
-    super.aroundReceive(around, msg)
+    around(msg)


pretty sure we must call super.aroundReceive somewhere, is this change safe?

Indeed. Have re-jigged to use super.aroundReceive as the zero for the fold to pick up the unhandled msg case

drewhk · 2015-06-16T07:24:59Z

OK TO TEST

akka-ci · 2015-06-16T08:38:50Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/2895/

ktoso · 2015-06-16T10:23:19Z

akka-contrib/src/main/scala/akka/contrib/pattern/ReceivePipeline.scala

@@ -38,10 +38,13 @@ trait ReceivePipeline extends Actor {
    val around = aroundCache match {
      case Some((`receive`, cached)) ⇒ cached
      case _ ⇒
-        val zipped = pipeline.foldRight[Receive](receive)((outer, inner) ⇒ outer(inner).orElse[Any, Unit](inner))
+        val zipped = pipeline.foldRight[Receive] {
+          case msg => super.aroundReceive(receive, msg)


I'm not sure if this is the exact semantics we need here, is it ok to call super.aroundReceive multiple times? Seems a bit weird.

You're right @ktoso, in a mixin context super.aroundReceive might not be just the default impl, and if any of the interceptors intentionally discard messages super.aroundReceive won't be called.

We should still use the original receive as zero but with fallback in unhandled. receive.orElse(PartialFunction(unhandled))

I saw that this was your first approach for fixing it @jeremystone. What was wrong with it that you changed it?

When @ktoso suggested that super.aroundReceive should be called somewhere, I agreed: what right has ReceivePipeline to enforce the specific use of unhandled as the fallback when the base actor might deliberately have overridden aroundReceive with some other behaviour.

Placing the super.aroundReceive call in the middle of the fold makes the fallback less invasive. But now as you point out, @ktonga, it might not get called at all. Clearly, calling super.aroundReceive at the end of the method as well will not do as then it will then often be called twice.

My gut feeling is that it is almost as if ReceivePipeline is ceding too much power over the flow control to the interceptors (allowing them to call each other directly). An alternative would be to have each interceptor as a PartialFunction[Any, InterceptorResult] with something like:

sealed trait InterceptorResult case class Inner(transformedMsg: Any) extends InterceptorResult case object HandledCompletely extends InterceptorResult

The ReceivePipeline would construct a Receive from these interceptors (so that it would be responsible for the message flow between interceptors) and just pass this to super.aroundReceive along with the original message at the end (as per the original implementation).

Of course this would be an API changer.

I like the idea @jeremystone, seems to be a definitive solution for the flow control problem. I don't think it is a problem to change the API since it is in the contrib module.

OK. I'd be happy to knock something up for this - and see how it looks.

ktoso · 2015-06-16T10:23:56Z

Would be nice @ktonga if you could have a look in this PR as well, since you authored the initial impl - thanks a lot in advance!

ktoso · 2015-07-03T14:25:47Z

So if I understand the discussion and code above correctly this only needs a rebase and is ready to be merged, right? (Would like to avoid the Merge branch commit)

akka-ci · 2015-07-03T17:27:05Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3129/

jeremystone · 2015-07-03T18:13:17Z

Just got round to pushing change addressing flow control as discussed, so
this needs review. Is a minor API change so need docs fixing up if it's OK.
On 3 Jul 2015 3:26 pm, "Konrad Malawski" notifications@github.com wrote:

So if I understand the discussion and code above correctly this only needs
a rebase and is ready to be merged, right?

—
Reply to this email directly or view it on GitHub
#17678 (comment).

ktonga · 2015-07-04T16:37:02Z

akka-contrib/src/main/scala/akka/contrib/pattern/ReceivePipeline.scala

+  private def toReceive(handler: Handler) = new Receive {
+    // Cache the result locally to avoid evaluating potentially 
+    // side-effecting code twice in isDefinedAt/apply...
+    var resultCache = Option.empty[(Any, HandlerResult)]


resultCache is never cleaned, it could unnecessarily prevent the message to be GCed. Maybe apply should empty it.

Good point.

Perhaps better approach is to not cache the result at all and simply override applyOrElse - as per recommendations for non-literal PFs. (If we can safely assume that the caller is aware of this and does not separately call isDefinedAt and then apply.) Namely:

private def toReceive(handler: Handler) = new Receive { def isDefinedAt(m: Any): Boolean = evaluate(m) != Undefined def apply(m: Any): Unit = evaluate(m) override def applyOrElse[A1 <: Any, B1 >: Unit](m: A1, default: A1 => B1): B1 = { val result = handler(m) if (result == Undefined) default(m) } private def evaluate(m: Any) = handler(m) }

jeremystone · 2015-07-05T06:54:41Z

Before we go any further with this, one thing that is missing (that could be achieved before) is the ability to do something within an interceptor after the message has been processed by the actor's receive (e.g. for the documented use case of timing message processing).

Looks like this could be provided by altering the Inner InterceptorResult to something like:

  case class Inner(transformedMsg: Any, after: Unit => Unit) extends InterceptorResult

along with some appropriate convenience factory methods providing defaults.

Then inside the combinedAdvisor ...

      outerInterceptor.andThen {
        case Inner(msg, after) =>
          val result = innerHandler(msg)
          after()
          result
        case HandledCompletely => Done
      }

Any thoughts on this?

ktoso · 2015-07-08T12:33:43Z

needs a try innerHandler() finally after() but looks good.

akka-ci · 2015-07-09T08:09:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3166/

ktoso · 2015-07-28T12:44:03Z

AFAICS this is ready right? If so, could you please squash it into one commit titling it =con #17670 Fix potential ReceivePipeline MatchError? Thanks @jeremystone !

jeremystone · 2015-07-28T16:52:04Z

Yes...so long as everyone happy with alterations to API. Will not be in a
position to squash the commit myself till the weekend.

There's also some documentation to do. Do you want this as part of the same
commit or as separate issue and pr?
On 28 Jul 2015 1:44 pm, "Konrad Malawski" notifications@github.com wrote:

AFAICS this is ready right? If so, could you please squash it into one
commit titling it =con #17670 Fix potential ReceivePipeline MatchError?
Thanks @jeremystone https://github.com/jeremystone !

—
Reply to this email directly or view it on GitHub
#17678 (comment).

ktoso · 2015-07-28T18:27:20Z

Docs in the same commit would be excellent :) Thanks!

akka-ci · 2015-08-02T15:02:15Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3312/

ktoso · 2015-08-02T21:34:00Z

akka-contrib/docs/receive-pipeline.rst

-for the messages of your interest and at some point delegate on the inner :class:`Receive`
-you get by parameter. We will talk about ignored and unhandled messages later.
+Multiple interceptors can be added to actors that mixin the :class:`ReceivePipeline` trait. 
+These interceptors can be thought of as layers: each outer interceptor advising the 


I'm not sure we should use the advice word, the API does not expose any "advice" in methods/types.
I know it's AOP wording and am familiar with it, but would like to avoid it leaking into the docs, since the API does not use such words.

OK. Could use decorate/decoration/delegate terminology as before, though the interceptor at the API level is now a more functional "description of the advice" than it was - so not sure this is right either unless this fact is made clear in the docs.
I will try to come up with an alternative.

Before I commit anything, how about the following which uses the decorator/delegate terminology and renames InterceptorResult to Delegation.

(Internally the code uses advice/advisor terminology - albeit privately - so will rename to line up and lessen any confusion here, too.)

Interceptors

Multiple interceptors can be added to actors that mixin the ReceivePipeline trait.
These interceptors internally define layers of decorators around the actor's behavior. The first interceptor
defines an outer decorator which delegates to a decorator corresponding to the second interceptor and so on,
until the last interceptor which defines a decorator for the actor's Receive.

[etc etc as before...]

Hi, and please excuse the delayed response!
Yes this sounds good to me, thanks!

akka-ci · 2015-08-16T13:33:09Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3418/

akka-ci · 2015-08-16T15:58:50Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3419/

ktoso · 2015-08-18T09:01:45Z

This is LGTM AFAICS, though needs to be squashed into one commit with title =con #17670 Fix potential ReceivePipeline MatchError

akka-ci · 2015-08-18T17:28:11Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://jenkins.akka.io/job/pr-validator-per-commit-jenkins/3446/

patriknw · 2015-08-20T09:31:40Z

LGTM
@ktoso can we merge this?

ktoso · 2015-08-20T09:35:26Z

Yes I think so, been LGTMed a lot ;-)

=con #17670 Fix potential ReceivePipeline MatchError

jeremystone · 2015-08-20T19:42:04Z

Cool!
On 20 Aug 2015 10:35 am, "Konrad Malawski" notifications@github.com wrote:

Merged #17678 #17678.

—
Reply to this email directly or view it on GitHub
#17678 (comment).

ktoso reviewed Jun 15, 2015
View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Jun 16, 2015

ktoso reviewed Jun 16, 2015
View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jul 3, 2015

ktonga reviewed Jul 4, 2015
View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Jul 9, 2015

ktoso added the t:core label Jul 15, 2015

ktoso added the reviewed Ready to merge from review perspetive, but awaiting some additional action (e.g. order of merges) label Jul 28, 2015

akka-ci added the validating PR is currently being validated by Jenkins label Aug 2, 2015

akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Aug 2, 2015

ktoso reviewed Aug 2, 2015
View reviewed changes

akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Aug 16, 2015

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Aug 16, 2015

=con #17670 Fix potential ReceivePipeline MatchError

b884dde

akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Aug 18, 2015

ktoso added a commit that referenced this pull request Aug 20, 2015

Merge pull request #17678 from jeremystone/wip-17670-jeremystone

d2f08a3

=con #17670 Fix potential ReceivePipeline MatchError

ktoso merged commit d2f08a3 into akka:master Aug 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

=con #17670 Fix potential ReceivePipeline MatchError #17678

=con #17670 Fix potential ReceivePipeline MatchError #17678

jeremystone commented Jun 6, 2015

akka-ci commented Jun 6, 2015

ktoso Jun 15, 2015

jeremystone Jun 15, 2015

drewhk commented Jun 16, 2015

akka-ci commented Jun 16, 2015

ktoso Jun 16, 2015

ktonga Jun 17, 2015

jeremystone Jun 17, 2015

ktonga Jun 18, 2015

jeremystone Jun 19, 2015

ktoso commented Jun 16, 2015

ktoso commented Jul 3, 2015

akka-ci commented Jul 3, 2015

jeremystone commented Jul 3, 2015

ktonga Jul 4, 2015

jeremystone Jul 4, 2015

jeremystone commented Jul 5, 2015

ktoso commented Jul 8, 2015

akka-ci commented Jul 9, 2015

ktoso commented Jul 28, 2015

jeremystone commented Jul 28, 2015

ktoso commented Jul 28, 2015

akka-ci commented Aug 2, 2015

ktoso Aug 2, 2015

jeremystone Aug 3, 2015

jeremystone Aug 3, 2015

ktoso Aug 14, 2015

akka-ci commented Aug 16, 2015

akka-ci commented Aug 16, 2015

ktoso commented Aug 18, 2015

akka-ci commented Aug 18, 2015

patriknw commented Aug 20, 2015

ktoso commented Aug 20, 2015

jeremystone commented Aug 20, 2015

=con #17670 Fix potential ReceivePipeline MatchError #17678

=con #17670 Fix potential ReceivePipeline MatchError #17678

Conversation

jeremystone commented Jun 6, 2015

akka-ci commented Jun 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewhk commented Jun 16, 2015

akka-ci commented Jun 16, 2015

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

ktoso commented Jun 16, 2015

ktoso commented Jul 3, 2015

akka-ci commented Jul 3, 2015

jeremystone commented Jul 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremystone commented Jul 5, 2015

ktoso commented Jul 8, 2015

akka-ci commented Jul 9, 2015

ktoso commented Jul 28, 2015

jeremystone commented Jul 28, 2015

ktoso commented Jul 28, 2015

akka-ci commented Aug 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Interceptors

Choose a reason for hiding this comment

akka-ci commented Aug 16, 2015

akka-ci commented Aug 16, 2015

ktoso commented Aug 18, 2015

akka-ci commented Aug 18, 2015

patriknw commented Aug 20, 2015

ktoso commented Aug 20, 2015

jeremystone commented Aug 20, 2015