Reverted: Convert non finite durations

[pjfanning]

Basically we have a bug in pekko.util.JavaDurationConverters.
It allows you to compile:

import pekko.util.JavaDurationConverters._
scala.concurrent.duration.Duration.Inf.asJava

But this will fail at runtime because pekko.util.JavaDurationConverters calls toNanos on the scala duration and this fails with

java.lang.IllegalArgumentException: toNanos not allowed on infinite Durations
	at scala.concurrent.duration.Duration$Infinite.fail(Duration.scala:219)

I found a case in pekko-http where we have a scala.concurrent.duration.Duration that we call asJava on and that could fail if the duration was set to a non-finite one (which is allowed).

Scala's builit-in converters for Durations only allow FiniteDuration instances to be converted while Pekko allows all Durations.

Java does not support Inifinite durations but this patch converts to the max allowed duration which is very long and safe enough.

Targetting 1.x as in 2.0.0, we will likely be deleting pekko.util.JavaDurationConverters

[He-Pin]

Myabe this can upstream to Scala/scala

[pjfanning]

Myabe this can upstream to Scala/scala

Scala didn't make the same mistake we made. They only have implicit support for asJava on FiniteDuration. Pekko added it for all Durations.

[He-Pin]

lgtm

[He-Pin]

maybe something like :

    if (duration.length == 0) JDuration.ZERO
    else duration.unit match {
      case TimeUnit.NANOSECONDS => JDuration.ofNanos(duration.length)
      case TimeUnit.MICROSECONDS => JDuration.of(duration.length, ChronoUnit.MICROS)
      case TimeUnit.MILLISECONDS => JDuration.ofMillis(duration.length)
      case TimeUnit.SECONDS => JDuration.ofSeconds(duration.length)
      case TimeUnit.MINUTES => JDuration.ofMinutes(duration.length)
      case TimeUnit.HOURS => JDuration.ofHours(duration.length)
      case TimeUnit.DAYS => JDuration.ofDays(duration.length)
    }

[pjfanning]

The existing solution is def asJava: JDuration = JDuration.ofNanos(self.toNanos) and this has not caused problems. I know that scala-compat-java8 uses something more like your code but this PR is not about issues with finite durations, it is about issues with non-finite durations (infinite and undefined).

[pjfanning]

@He-Pin @mdedetrich @raboof I'm not sure whether this change helps much. The code with the bug (not being able to handle infinite or undefined Scala durations) is already inlined into our module jars.
We haven't hit any issues yet but maybe it is safer to put this fix in, as at least it covers the code in this repo.
I played around with long finite durations and found that the existing code for finite durations is adequate (just using nanos).
I added comments in the code about the fact that converting from a Java Duration to a Scala Duration can fail with an IllegalArgumentException. Scala FiniteDurations max out at plus or minus 252 years while Java Durations support billion year durations. This is a pretty unlikely scenario.
We do see Duration.Inf used in Scala though as it is relatively commonly used by people who don't want to apply a limit.

[He-Pin]

Technically speaking, this change is correct now, but it may not be a problem in real scenarios. At least I haven't seen anyone report any problems in pekko or akka.

[mdedetrich]

@He-Pin @mdedetrich @raboof I'm not sure whether this change helps much. The code with the bug (not being able to handle infinite or undefined Scala durations) is already inlined into our module jars. We haven't hit any issues yet but maybe it is safer to put this fix in, as at least it covers the code in this repo. I played around with long finite durations and found that the existing code for finite durations is adequate (just using nanos). I added comments in the code about the fact that converting from a Java Duration to a Scala Duration can fail with an IllegalArgumentException. Scala FiniteDurations max out at plus or minus 252 years while Java Durations support billion year durations. This is a pretty unlikely scenario. We do see Duration.Inf used in Scala though as it is relatively commonly used by people who don't want to apply a limit.

@pjfanning

Sorry I just noticed this now. Actually I am not that supportive of merging this in the 1.x series of Pekko because it can create really confusing behaviour with inlining (as you have pointed out) and I it hasn't been causing any problems in actual production (I understand that its easy to create a scenario where this can break but thats different to people experiencing it in prod).

Merging this PR for 2.0.x is absolutely fine, but with how inlining works for the 1.x series of Pekko, we can't even announce it as a normal bug fix because replacing the pekko jar wouldn't work, they would have also to replace every pekko satellite project that the user happens to be running.

What are your thoughts about reverting this change? I can still see your argument so I am not going to push strongly, but it is a really confusing experience for end users.

[pjfanning]

Thanks @mdedetrich. I'm away from my computer today. I'll think about reverting this and will attempt to justify it otherwise.
For 1.x, the code in this repo benefits from it. I found 3 affected cases in pekko-http and have created a pekko-http specific class to handle the issue there. The changes are in main branch of pekko-http only. I will check over the next few days if any affected code is at risk of trying to convert non-finite durations.

[mdedetrich]

Thanks @mdedetrich. I'm away from my computer today. I'll think about reverting this and will attempt to justify it otherwise. For 1.x, the code in this repo benefits from it. I found 3 affected cases in pekko-http and have created a pekko-http specific class to handle the issue there. The changes are in main branch of pekko-http only. I will check over the next few days if any affected code is at risk of trying to convert non-finite durations.

I would say that if we keep this PR, we would need to do a new release for every single Pekko satellite project because thats the only way we can actually state that this fix would work as it would only inline at compile time. Not saying that we should do all releases immediately, but we need to plan about doing at least one such release per satellite project. Its also a bit annoying as Pekko Connectors itself just had a 1.2.0 release, but it is what it is.

[He-Pin]

@pjfanning So the inline is been enabled in Pekko 1.2?

[mdedetrich]

@pjfanning So the inline is been enabled in Pekko 1.2?

Inlining has been enabled since 1.1.x (iirc)

[He-Pin]

Opps。

[pjfanning]

So far, I've only seen the issue in pekko-http - so one solution is that in pekko-http 1.3.0 that we fix the problematic conversions. They are fixed in main branch already.