Include cause in io.CommandFailed?

[patriknw]

imported from https://www.assembla.com/spaces/akka/tickets/3861

Question/request by Horst Dehmer (can't find it in the archives):

I'm playing with akka-io (2.2.3).

I'm a little confused about the CommandFailed message, which obviously doesn't contain an optional cause (read Throwable), why a certain command failed. With UDP, for example, a Bind() command can fail for a number of reasons: (BindException: Address already in use, SocketException: Permission denied, ...) akka.io.UdpListener:51 just logs the exception and issues a CommandFailed().

Why is it that the underlying cause is not forwarded to caller (in my case some Bootstrap actor)? Is that an oversight in the design, or do I misunderstand something?
How am I supposed to report the underlying cause to a end user?

I probably could clone and shadow the original UdpListener and roll my own CommandFailed(), but that seems rather strange. What could be wrong with something like CommandFailed(Command, Option[Throwable])?

I'd appreciate some clarification on this topic.

[RichardBradley]

I have also found this surprising and difficult to work with.

A relevant comment from the old ticket: " jrudolph: A related problem is that e.g. akka.io.TcpListener logs errors during binding (and others, as well) just with DEBUG level so that it is even harder to see errors."

[rkuhn]

The reason was—IIRC—that the errors you get back are highly O/S dependent and therefore not that interesting programmatically. But I agree that the desire to log them is enough to warrant their inclusion.

[ivan-lorenz]

Hi,

I'm with this issue. Adding the "throwable" cause to CommandFailed means breaking binary compatibility (I have 13 alerts from MiMa). This is a public case class (3 to be more accurate) so it's going to hurt current production code.

Should I add MiMa filters for these alerts? Does it means that we cannot release in 2.4.x?

Thank you in advance.

[ktoso]

It's not possible to just add a parameter like that to a case class in a binary compatible way.
It is possible to work around this by making a 2ndary parameter list, which does not participate in case class extraction If I remember correctly.

This needs a bit of toying around to see if it can be made binary compatible.

If it's not possible, the first point in time where we can break compatibility is Akka 3.0, please read the binary compatibility guidelines to see why http://doc.akka.io/docs/akka/2.4/common/binary-compatibility-rules.html (2.3 is compatible with 2.4, which will be compatible with 2.5)

[ivan-lorenz]

Thanks Konrad.

I will try the 2ndary parameter list thing. I will catch up with you to check if this will work.

[ivan-lorenz]

I've tried this:
final case class CommandFailed (cmd: Command)(val throwable: Option[Throwable]) extends Event
object CommandFailed {
def apply(cmd: Command) = new CommandFailed(cmd)(None)
}

But it is not going to be so easy :)

We are having "ambiguous reference to overloaded definition" when we try to create an object with one parameter.

I've found a work-around to keep compatibility and be able to let the code create CommandFailed with one parameter and optionally adding a second parameter list with a Throwable. Also unapply is working for only one parameter. It has several drawbacks:

1. Class must be not final and not case class,
2. So we need to implement hashCode() and equals() ouch...!
3. We need to implement "Serializable".
4. To use the extractor with the new Throwable we need to pattern match with a new object:
   object CommandFailedWithThrowable {
def unapply(arg: CommandFailed) = Some((arg.cmd, arg.throwable))
}

What do you think? Should we continue with this solution or must we break compatibility?

[ivan-lorenz]

Nope. Trying to implement the work-around is worst. MiMa fails with even more alerts detecting CommandFailed hierarchy has been modified.

I will continue trying to find a way keeping CommandFailed as a case class while maintaining compatibility. If I can't it will be a feature for 3.x.

[RichardBradley]

the first point in time where we can break compatibility is Akka 3.0, please read the binary compatibility guidelines to see why http://doc.akka.io/docs/akka/2.4/common/binary-compatibility-rules.html (2.3 is compatible with 2.4, which will be compatible with 2.5)

Slight derail, but that page explains in detail the binary compatibility policy and how it is enforced, but I can't see any discussion in there of why it is necessary? Do you have a doc link or a link to a previous discussion that covers that?

Requiring a recompile when changing a minor version of a dependency (i.e. breaking binary compat) seems to be fairly standard in other libs, so I imagine there must be a compelling reason for this extra constraint on Akka?

[drewhk]

In short: people in production. Our policy ensures that they only need to replace the JARs on the classpath to fix an issue without recompile.

[RichardBradley]

In short: people in production. Our policy ensures that they only need to replace the JARs on the classpath to fix an issue without recompile.

Do many users expect that? I find that surprising.

I know that I wouldn't attempt to change a dependency of an app in production without retesting, which usually would mean a recompile + a rebuild + a "verify" using CI etc.
I suppose now that Akka has offered this guarantee, it must meet it, but it seems that the extra custom tooling it has required to enforce this guarantee is a clear indication that this is not a standard requirement.

I'd be interested to see any evidence that people rely on this, or known scenarios in which this is useful. It feels to me like saying that this guarantee exists so that users can fix an issue without recompile is a restatement of what the binary compatibility policy offers, rather than an explanation of why it is valuable.

Maybe there is no more "why" than just that Akka offers it and people have come to rely on it. :-)

[ktoso]

Hi Richard, Everyone relies on this all the time.

It's not for people randomly switching JAR files on production, that's a bad idea.
It IS however for being use any kind of library with Akka.

Scenario is this:

• Someone builds library for Akka 2.4.8. It works.
• You're on 2.4.8, it works.
• You depend on that library, it just works.
• You decide to upgrade Akka to 2.4.9.
• Author of lib did not have re-compile for 2.4.9, because why should they – 2.4.x are all binary compatible (hah, even all 2.x are compatible nowadays (where x > 2)).
• It just works.

If we dropped binary compat, you'd be in hell every time you upgrade anything.
The entire ecosystem would have to compile everything again and again.

Requiring a recompile when changing a minor version of a dependency (i.e. breaking binary compat) seems to be fairly standard in other libs, so I imagine there must be a compelling reason for this extra constraint on Akka?

You must be referring to someone's pet project. Compatibility is crazy important - seriously maintained projects keep a keen eye on this. Even more so for a toolkit / library like Akka which lives at the very core or multiple large projects or libraries. These projects MUST be able to upgrade to the latest version possible without being stuck because some other dependency didn't recompile for the newer version.

Thus: We MUST keep binary compatibility, it is a feature.
We will only drop it given serious reasons, and only in Akka 3.

[ivan-lorenz]

Hi,

back to work :)

I finally achieve using the curried form of CommandFailed:
final case class CommandFailed private[CommandFailed] (cmd: Command)(val throwable: Option[Throwable]) extends Event

The tests are green, but I needed to implement the copy method and a pair of apply in the companion object. MiMa is now complaining only with this two alerts:
[error] * method this(akka.io.Tcp#Command)Unit in class akka.io.Tcp#CommandFailed does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("akka.io.Tcp#CommandFailed.this")
[error] * the type hierarchy of object akka.io.Tcp#CommandFailed is different in current version. Missing types {scala.runtime.AbstractFunction1} [error] filter with: ProblemFilters.exclude[MissingTypesProblem]("akka.io.Tcp$CommandFailed$")

The second error is due to object companion of curried form case classes not implementing AbstractFunctionN. But this should be ok if we don't need to use "tupled" and "curried" methods. Anyway I will try to remove both alerts after holidays. Also I want to add a test for udp errors returning CommandFailed with a throwable cause.

But is the first error that annoys me. Is complaining about the "this" method. Any advice/alert/experience on this?

[ktoso]

The second we usually solve by extending in the companion directly the AbstractFunction1 trait and implementing it manually.

The first I think should be solvable by making a single argument constructor and delegating to the double one? Not sure, this may be tricky.

[RichardBradley]

@ktoso -- thanks, that makes a lot more sense. I wasn't thinking this through.
I forgot all about "Dependency Convergence"

It's not for people randomly switching JAR files on production, that's a bad idea.

Agreed -- that explanation didn't really hold water for me. Your reply explains it all, thanks.

If we dropped binary compat, you'd be in hell every time you upgrade anything.
The entire ecosystem would have to compile everything again and again.

Not that it makes much difference to the principle, but the ecosystem does seem to be recompiling everything Akka related with every point release at present. On our fairly large commercial Akka-based project, we didn't have any trouble getting full dependency convergence every time we upgraded to a new Akka point release.

I'll try to stop derailing this issue now, thanks all.

[jrudolph]

Fixed for TCP CommandFailed in #22954.