Skip to content

FSM transform...using pattern double executes transform piece #30855

@kubudi

Description

@kubudi

Hello everyone,

I know FSM is deprecated and not encouraged to use on new projects. But believe it or not some people are still using it.

While recently trying to update to scala 2.13 we realized concat on partial functions has changed behaviour. Basically if you andThen two partial functions, and you use isDefinedAt over the chain, you execute the first partial function twice. Here is a really simple example showing this behaviour:

  scala> val pf1:PartialFunction[Int, String] = { case 3 => println("here in pf1"); "3"}
  val pf1: PartialFunction[Int,String] = <function1>
  
  scala> val pf2:PartialFunction[String, Unit] = { case "3" => println("here in pf2")}
  val pf2: PartialFunction[String,Unit] = <function1>
  
  scala> val chain = pf1 andThen pf2
  val chain: PartialFunction[Int,Unit] = <function1>
  
  scala> if(chain.isDefinedAt(3)) chain(3)
  here in pf1
  here in pf1
  here in pf2

Please check this pr for more details:
scala/scala#7263

This breaks FSM's transform...using pattern.

  final class TransformHelper(func: StateFunction) {
    def using(andThen: PartialFunction[State, State]): StateFunction =
      func.andThen(andThen.orElse { case x => x })
  }

  final def transform(func: StateFunction): TransformHelper = new TransformHelper(func)

You will see here it uses andThen on partial function. And if we check the implementation you will see there is actually a warning there:

/**
   * Composes this partial function with another partial function that
   * gets applied to results of this partial function.
   *
   * Note that calling [[isDefinedAt]] on the resulting partial function may apply the first
   * partial function and execute its side effect. It is highly recommended to call [[applyOrElse]]
   * instead of [[isDefinedAt]] / [[apply]] for efficiency.
   *
   * @param  k  the transformation function
   * @tparam C  the result type of the transformation function.
   * @return a partial function with the domain of this partial function narrowed by
   *         other partial function, which maps arguments `x` to `k(this(x))`.
   */
  def andThen[C](k: PartialFunction[B, C]): PartialFunction[A, C] =
    new Combined[A, B, C](this, k)

But FSM uses isDefinedAt on the resulting PartialFunction. For Example:

  private[akka] def processEvent(event: Event, @unused source: AnyRef): Unit = {
    val stateFunc = stateFunctions(currentState.stateName)
    val nextState = if (stateFunc.isDefinedAt(event)) {
      stateFunc(event)
    } else {
      // handleEventDefault ensures that this is always defined
      handleEvent(event)
    }
    applyState(nextState)
  }

One solution comes to mind is to use applyOrElse as the documentation dictates. Something like:

  private[akka] def processEvent(event: Event, @unused source: AnyRef): Unit = {
    val stateFunc = stateFunctions(currentState.stateName)
    val nextState = istateFunc.applyOrElse(event,  handleEvent)
    }
    applyState(nextState)
  }

But I am not sure if that's enough.

We are planning to move away from FSM as well, but in the meantime, it would be great to have a fix for this.

Please let me know your ideas. If you think this solution is valid and enough, I can open a pull request for it.

Thanks!

P.S. We are using 2.6.14 but I couldn't see any mentions of this in later releases.
I also couldn't find any mentions of this on other issues. Please let me know if this is a duplicate.

Metadata

Metadata

Assignees

No one assigned

    Labels

    1 - triagedTickets that are safe to pick up for contributing in terms of likeliness of being accepted

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions