add util method to check that StackOverflowError is reproducible

[nikitapecasa]

This can be useful to make sure that stack-safety tests are actually reproducing SOE with corresponding stack depth.

If you like the idea, I can add the same check for IOspec

[vasilmkd]

Thanks for opening this PR.

I'm honestly against this change, firstly because StackOverflowError is an Error and not an Exception and my understanding is that Errors should not be caught and recovered from because the JVM is in an undefined state after they occur, which can further manifest in strange ways when running the other tests.

Second, it seems that Scala.js does not like this change at all, probably for similar reasons, and this even happens with quickly optimized code, which is in general better at recognizing and recovering from exceptions. Fully optimized Scala.js might have even bigger stability issues.

The following excerpt is taken directly from the NonFatal Throwable extractor defined in Scala, which is what we heavily rely on in this project.

/**
 * Extractor of non-fatal Throwables. Will not match fatal errors like `VirtualMachineError`
 * (for example, `OutOfMemoryError` and `StackOverflowError`, subclasses of `VirtualMachineError`), `ThreadDeath`,
 * `LinkageError`, `InterruptedException`, `ControlThrowable`.
 */

with the code being the following:

object NonFatal {
   /**
    * Returns true if the provided `Throwable` is to be considered non-fatal, or false if it is to be considered fatal
    */
   def apply(t: Throwable): Boolean = t match {
     // VirtualMachineError includes OutOfMemoryError and other fatal errors
     case _: VirtualMachineError | _: ThreadDeath | _: InterruptedException | _: LinkageError | _: ControlThrowable => false
     case _ => true
   }
  /**
   * Returns Some(t) if NonFatal(t) == true, otherwise None
   */
  def unapply(t: Throwable): Option[Throwable] = if (apply(t)) Some(t) else None
}

[nikitapecasa]

@vasilmkd thanks for the prompt response, really appreciate it :) as well as honest feedback.
I've been thinking about those issues too, but didn't find any proof that it would leave JVM in a corrupted state (https://openjdk.java.net/jeps/270).
For Scala.js case maybe the check could be skipped somehow, tbh have no idea how to address it there.

To me personally there's an open question if this PR is not merged, how can we be sure that stack-safety tests are actually working and would fail if stack-safety is broken?
I see that 10000 is used in IOSpec and ResourceSpec, and it looks it is not enough to trigger SOE there (might be wrong though).

Please don't get me wrong, I'm not trying to insist on merging this PR :)
I'm just solving the same problem for another library on github, and looking for some good way to test stack safety on JVM.

Edit: found a suggestion to read https://pangin.pro/posts/stack-overflow-handling ( reading now :) )

[vasilmkd]

I read the JEP, and while I really appreciate the effort, it has 2 "problems" that are IMO deal breakers for Cats Effect. The JEP is explicitly said to not work on Windows, which is a platform we officially support and explicitly test on. The second is that this JEP arrived in Java 9, and we absolutely must support Java 8.

Again, I'm not saying that this is not a good effort, far from it. However, I still feel uncomfortable including this in our build pipeline and potentially destabilizing the build by making assumptions on undocumented behavior. With that being said, if other contributors do not share my opinions, I will not oppose this addition. 😄

[nikitapecasa]

@vasilmkd Totally understand and respect your decision :)

What do you think about bumping the depth to 50000 in some tests (instead of current 10000)
it looks like it is hitting the SOE in ResourceSpec "use is stack-safe over binds - 2" with 50000.

I was also asking the same question in telegram channel (sorry it's a Russian speaking one), and got some explanations from Aleksey Shipelev there (https://shipilev.net), and IIUC it should not break the JVM.

[vasilmkd]

That's awesome!

So if I understand correctly, the worst thing that can happen is that the thread will just die?

[nikitapecasa]

the thread will just die

@vasilmkd If I understand correctly - yes it's the worst thing that can happen in such case, but I'm not an expert in JVM,
and totally understand that stability of the build might be more reasonable choice here :)

Also I updated my previous comment too late (after your response, sorry about that), so I would repeat that small question again :)

What do you think about bumping the depth to 50000 in some tests (instead of current 10000)
it looks like it is hitting the SOE in ResourceSpec "use is stack-safe over binds - 2" with 50000.

Edit here are some proofs that 10000 was not enough for both JS and JVM envs

java.lang.RuntimeException: expected a StackOverflowError from 10000-deep recursion, consider increasing the depth in test (file:///home/runner/work/cats-effect/cats-effect/tests/js/target/scala-2.12/cats-effect-tests-test-fastopt/main.js:38429)

! use is stack-safe over binds
[error]    java.lang.RuntimeException: expected a StackOverflowError from 10000-deep recursion, consider increasing the depth in test (ResourceSpec.scala:993)

[nikitapecasa]

About JEP 270 - I didn't mean that it's making this PR safer in any way (sorry for any confusion), it was my first closest finding on the topic, so we don't need it for this PR, as a result Java 9 and Windows support are not relevant.
But, again :) I understand uncomfortable feelings about such PR, I feel it too :)
and it's way easier for me to have it in one small not yet so popular scala library vs cats effect, so no hard feelings from my side, and actually I learned even more on the topic while working on this PR.

[djspiewak]

I actually really like the idea here. I need to study it a bit more but, given that it's in tests, it doesn't seem horrible. SOE definitely doesn't leave the JVM in an undefined state (unlike OOM), so it's not totally corrupting things.

[joroKr21]

I think the best you can do on Scala.js is check the error message and I couldn't figure out if it's even possible on Scala Native.

[djspiewak]

I think the best you can do on Scala.js is check the error message and I couldn't figure out if it's even possible on Scala Native.

Some JavaScript runtimes have automatic tailcall elimination in certain common circumstances, likely including this one. As an example, I've had difficulty getting V8 to stackoverflow without significantly obfuscating the recursion. It usually just runs out of memory instead.

[vasilmkd]

@nikitapecasa sorry for the delay on this issue. I think there is merit to this change, and in order to get the most of it while avoiding unnecessary complexity to trick node/Scala.js, let's just make this test platform specific for the JVM only.

[vasilmkd]

So 50000 isn't enough? I'm confused.

[vasilmkd]

This is ready for merging.

[nikitapecasa]

@vasilmkd well done! I really like how you changed the method to actually reach SOE instead of trying to guess it :)
and sorry that I was away for so long (too much personal stuff going on).

I have a few questions though

• is there anything else we should do in scope if this PR to use the same technique in laws testing?
• is there anything else I can help with to finish this PR (like review/code changes)?

[vasilmkd]

The laws need to work on Scala.js. Personally, I think it's fine to leave them as is. I'll let other maintainers decide.

[djspiewak]

Thank you for chasing this!