parallel and sequential computations are not equivalent (up to failure messages) in some cases

[tpolecat]

Ciris is very good. I am enjoying it!

I have run into some unexpected (to me) behavior regarding the interaction of missing properties and sequential vs. parallel composition. In the following code I have two configs that differ only in composition mode. I would expect that both will succeed or both will fail, the difference being only in the error message on failure. However this is not the case. config2 fails to load even though there is a default alternative provided. This seems like a bug to me.

import cats.effect._
import cats.implicits._
import ciris._

object CirisTest extends IOApp {

  val config1 = (prop("a"), prop("b")).tupled    or default(("foo", "bar"))
  val config2 = (prop("a"), prop("b")).parTupled or default(("foo", "bar"))

  def run(args: List[String]): IO[ExitCode] =
    for {
      _ <- IO(System.getProperties().put("a", "A"))
      _ <- config1.load[IO].flatMap(c => IO(println(s"config1 is $c")))
      _ <- config2.load[IO].flatMap(c => IO(println(s"config2 is $c")))
    } yield ExitCode.Success

}

Output looks like this.

config1 is (foo,bar)
[E] ciris.ConfigException: configuration loading failed with the following errors.
[E] 
[E]   - Missing system property b.
[E] 
[E]     at ciris.ConfigException$.apply(ConfigException.scala:57)
[E]     at ciris.ConfigError.throwable(ConfigError.scala:114)
[E]     at ciris.ConfigValue.$anonfun$load$1(ConfigValue.scala:176)
[E]     at cats.effect.Resource.$anonfun$fold$1(Resource.scala:118)
...

I experimented a bit and found that this only happens if the failure in config2 is partial. If both system properties are missing that config2 succeeds (i.e., if you change it to (prop("b"), prop("b")).parTupled ....

[vlovgr]

I'm glad you're enjoying the library, and thanks for raising the issue.

While I agree the different behaviours can be confusing, both work as intended. Since ConfigValue has a FlatMap instance, we're limited to a tail-recursive flatMap (due to the tailRecM requirement). This means that prop("a") cannot affect prop("b") if the first loads successfully, as is the case here for config1.

However, for ConfigValue.Par, we're limiting ourselves to Apply, and as such are not limited by the tail-recursive requirement. So, for parallel composition prop("a") can affect prop("b") even if the first loads successfully. Essentially, we're using this to keep track of whether something has been loaded or not, and choose to not use default values if that's the case.

In the case of config2, we interpret the situation as: you specified a but forgot b, so we're making you aware of that, in case it was an error on your side, rather than using the default value. If you want default values to be used as long as something is not specified, you can rewrite to the following.

(
  prop("a").default("foo"), 
  prop("b").default("bar")
).parTupled

Ideally, we would change flatMap to a non-tail-recursive version, and make it consistent with parallel composition, but then we would technically not have a lawful FlatMap instance. I've seen there is already some support for testing stack unsafe monads in cats-laws, so maybe that's fairly accepted already?

[tpolecat]

Hi, thanks for your note.

This strikes me as weird because a or b doesn't mean "if a fails then try b" ... it means if a fails and has been constructed in such a way as to allow for it, then try b.

So, basically I expect it to behave like Either:

// error cases are distinct, as expected

@ ("a".asLeft[Int], "b".asLeft[Boolean], "foo".asRight).tupled 
res5: Either[String, (Int, Boolean, String)] = Left("a")

@ ("a".asLeft[Int], "b".asLeft[Boolean], "foo".asRight).parTupled 
res6: Either[String, (Int, Boolean, String)] = Left("ab")

// but recovery and success are indistinguishable

@ ("a".asLeft[Int], "b".asLeft[Boolean], "foo".asRight).tupled orElse Right((42, true, "bar")) 
res7: Either[String, (Int, Boolean, String)] = Right((42, true, "bar"))

@ ("a".asLeft[Int], "b".asLeft[Boolean], "foo".asRight).parTupled orElse Right((42, true, "bar")) 
res8: Either[String, (Int, Boolean, String)] = Right((42, true, "bar"))

I will have a look at the implementation and see if I have any ideas.

Also if it were up to a vote I would jettison stack safety since it seems unlikely that I will have many thousands of flatMaps in my configuration, but that's just me ;-)

[vlovgr]

There’s already parFlatMap on ConfigValue, so if we make flatMap equal to that, they should be consistent.

[vlovgr]

I've raised #376 with flatMap being the same as parFlatMap, so sequential and parallel composition should now be the same (up to error messages). Both config1 and config2 in your example now fail with 'missing system property b', and additional errors accumulate with parTupled but not tupled, as expected.

As you note, a.or(b) doesn't mean 'if a fails, use b' but rather 'if a is missing, use b'. For example, we wouldn't want to use default values when a value was available but deserialization failed. And similarly, we also say we don't want to use default values when parts of a composition was loaded but some parts were not (to avoid the mistake of specifying a partial configuration which then gets ignored completely by the default value).

[vlovgr]

#376 released as part of v1.2.1.