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.

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

Possible problems with typed adapters #31393

Open
johanandren opened this issue May 10, 2022 · 5 comments
Open

Possible problems with typed adapters #31393

johanandren opened this issue May 10, 2022 · 5 comments
Labels
0 - new Ticket is unclear on it's purpose or if it is valid or not t:typed

Comments

@johanandren
Copy link
Member

Noticed by @leviramsey :

Message adapters are kept in a list which is prepended to after filtering out any created adapter for the exact same incoming message type.

When applying the adapters, it is traversed from most recent and backwards, checking if the incoming message is assignable from the class for each adapter.

This means you can register multiple adapters from the same class hierarchy and get different behavior based on the order the adapters were spawned in, so these two snippets will adapt differently:

val adapterA = ctx.messageAdapter[AnyRef](a => ...)
val adapterB = ctx.messageAdapter[MyClass](m => ...)
val adapterB = ctx.messageAdapter[MyClass](m => ...)
val adapterA = ctx.messageAdapter[AnyRef](a => ...)

Maybe this is fine, if you create many adapters which match further and further up in the class hierarchy, that will keep a bunch of adapters around that will never be used. But on the other hand we recommend to not create a ton of adapters in the docs.

We could perhaps look for earlier adapters assignable from the latest to filter such out as well?

Not sure if there is some more tricky consequence of this that I'm not seeing?

@johanandren johanandren added 0 - new Ticket is unclear on it's purpose or if it is valid or not t:typed labels May 10, 2022
@He-Pin
Copy link
Member

He-Pin commented May 11, 2022

What if it's not matched with the predicate? I think IDEA and scalac do some checks too.

@leviramsey
Copy link
Contributor

Since messageAdapter requires a total function, an adapter which doesn't match the incoming message would have to throw, which (since the adaptation runs inside the actor) would crash the actor.

@patriknw
Copy link
Member

It would be nice to sort the adapters with most specific first. Not sure if that can be done?
For MyClass and AnyRef and for an incoming message of MyClass or subclass it would pick that matching adapter, but for other messages the AnyRef adapter, independent of in what order they were registered.

@leviramsey
Copy link
Contributor

One could probably define an Ordering[Class[_]] based on assignability with something like:

// only mentally compiled...
new Ordering[Class[_]] {
  def compare(x: Class[_], y: Class[_]): Int = {
    val xAssignableToY = y.isAssignableFrom(x)
    val yAssignableToX = x.isAssignableFrom(y)
    
    (xAssignableToY, yAssignableToX) match {
      case (true, true) => 0  // same class
      case (false, true) => 1  // x is a supertype of y
      case (true, false) => -1 // y is a supertype of x
      case (false, false) => 0 // arbitrary: no relation between the class objects
     }
  }
}

Then we could maybe use a SortedList to store this, depending on how much information about the sort results is retained in SortedList because this ordering isn't transitive. But a collection could be implemented such that if we insert a class, it will be iterated before any other class to which it's assignable (we'd definitely not want to use an immutable List for that!).

That said, that approach is a behavior change: keeping the same structure but removing elements based on assignability wouldn't change behavior but save a bit of memory.

@He-Pin
Copy link
Member

He-Pin commented May 13, 2022

refs: #22849 too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - new Ticket is unclear on it's purpose or if it is valid or not t:typed
Projects
None yet
Development

No branches or pull requests

4 participants