Monad defer laws #1150
Monad defer laws #1150
Conversation
|
Tests will fail btw because of the one DeferredK test (or they better should :D) |
modules/core/arrow-test/src/main/kotlin/arrow/test/laws/MonadDeferLaws.kt
Outdated
Show resolved
Hide resolved
|
Good job so far! The anwers to your questions: Flatten is in Monad, which means it's available in MonadDefer: https://github.com/arrow-kt/arrow/blob/master/modules/core/arrow-typeclasses/src/main/kotlin/arrow/typeclasses/Monad.kt#L22 Defer constant is a new function you can add in MonadDefer: RaiseError is in ApplicativeError, of which MonadDefer inherits too: https://github.com/arrow-kt/arrow/blob/master/modules/core/arrow-typeclasses/src/main/kotlin/arrow/typeclasses/ApplicativeError.kt#L8
Yeah, it's likely a duplicated, but keep it just in case :D
*> is a shorter version of flatMap, just use flatMap instead
If the instances or the implementations are broken, we need to check the cause 🔍
Seems like a bug to fix to to me caught by the tests then hahaha |
|
So is a function
Well they don't have a delay/defer suspends eval law so that might it. So add it regardless?
Well other instances (IO) override delay as well, to delegate it to invoke which in turn does exactly the same as delay in MonadDefer except it does not catch errors. And for async and invoke I am guessing it should just start lazy instead of default. I am just asking all of these questions because there might have been a reason to implement it like that 🙈 |
Yeah, that's it, ez :D
yeah, go for it
Yep!
So if I understand correctly, the cause of the test failing is that one of invoke/suspend/defer is not catching errors in the block when it's supposed to. Easy fix, try/catch the implementations in IO.kt then :D |
Not quite, the cause of the test failing is that DeferredK's async instance overrides delay which calls invoke which starts default instead of lazy. I was just bringing this up to understand why delay was overriden there in the first place |
|
Yes, it seems incorrect. Just change it to the original implementation in the datatype then. |
Monad defer already provides a default for it. And if async override differs in any way this would produce unexpected results
|
So I added repeatedSyncEvaluationNotMemoized not sure how useful it is tho. I skipped unsequencedDelayIsNoop because it ends up exactly like delaySuspendsEvaluation. Regarding stack safety. Are all MonadDefer instances supposed to be stack safe? |
Codecov Report
@@ Coverage Diff @@
## master #1150 +/- ##
===========================================
+ Coverage 42.62% 42.9% +0.28%
- Complexity 820 836 +16
===========================================
Files 404 404
Lines 11355 11417 +62
Branches 1272 1273 +1
===========================================
+ Hits 4840 4899 +59
- Misses 6125 6129 +4
+ Partials 390 389 -1
Continue to review full report at Codecov.
|
yep. |
|
Then there is a problem with reactor and rx2 instances of MonadDefer because they all fail the tests (I'll push them in a sec) for stacksafety while IO and DeferredK work just fine Edit: Some more info. They each fail with a stackoverflow error (which gets swallowed by the eq instance) and since a stackOverflow error does not equal just(number) the test fails... |
|
@1Jajen1 any way to see where the stack is recursing? |
|
@raulraja For reactor (MonoK) the stacktrace points to methods from reactor. This happens when ~5000 flatMaps get evaluated StackTrace excerpt: The last two lines get cycled over and over again. FluxK: And this as well cycles on and on... I think rx2 will be similar. I don't really know what to do with this, or if something from inside arrow can/should be done. |
| val sideEffect = SideEffect() | ||
| val df = delay { sideEffect.increment(); sideEffect.counter } | ||
|
|
||
| df.flatMap { df }.flatMap { df }.equalUnderTheLaw(just(3), EQ) |
There was a problem hiding this comment.
shouldBe true, else this is not checked
There was a problem hiding this comment.
Can't believe I forgot that 😅. Added it and it works for io, reactor and rx2 but fails for DeferredK.
There was a problem hiding this comment.
That is a good question and might be an issue or feature of coroutines. Will test this more thoroughly later on.
A possible scenario would be that coroutines simply after finishing return their result instead of rerunning when awaited again
There was a problem hiding this comment.
@pakoito
Well it makes sense now... Thats just how coroutines work:
var sideEffect = 0
val a = GlobalScope.async(start = CoroutineStart.LAZY) {
++sideEffect
}
println("Started a: ${a.start()}")
runBlocking {
println("Result a: ${a.await()}")
}
val b = GlobalScope.async(start = CoroutineStart.LAZY) {
a.await()
}
println("Started b: ${b.start()}")
runBlocking {
println("Result b: ${b.await()}")
}
println(sideEffect)Which prints:
Started a: true
Result a: 1
Started b: true
Result b: 1
1
So what should be done about this? This is not like the stack-safety issue, if someone were to try and build a retry/repeat scheduler that won't work with DeferredK. Also other code which reuses MonadDefer instances (and then gets a deferredK) simply won't work. And the more annoying problem here is that it would be very hard to track down to Coroutines in that case :/
I don't really know how to solve this, wrapping deferred in DeferredK into a function has other challenges (stack-safety etc) that might not work... Maybe someone at jb knows what to do here...
There was a problem hiding this comment.
Right, I forgot they are memoized -.- This cannot be solved by JBs as it'd require moving the stack labels, which is the same issue we have with monad comprehensions, and it's just a design choice instead.
cc @raulraja for ideas
There was a problem hiding this comment.
Ok so how about I create a ticket for this. Comment the test out for now with a fixme linked to the ticket, add a copy to io's tests (also with a comment and link) and let that ticket deal with it then.
There was a problem hiding this comment.
let's wait for opinion, this one is somewhat different and may be solved with design changes by wrapping in functions
|
I've opened a ticket on the Project Reactor and Rx2 repos, let's wait for a reply. |
|
@pakoito can you link them here? |
|
It seems like it isn't happening. After talking to Raul, it's better to remove the tests rather than the instances. Maybe leave them commented out with a |
|
Welp yeah will do. Should that be added to the docs somewhere? (better yet create a ticket for it since that is not really the scope of this pr...) |
|
Nah, leave it as the commented out tests with that FIXME. We have a couple of those in the codebase already. As there'll be no fix on the Rx/Reactor department, not much we can do except wait for the first complaint then point at IO, Deferred or arrow-streams. |
|
Not mandatory for this ticket, but if you could add the FIXME to the tailrecM implementations of Observable, Flowable, Mono, and Flux here we'd save ourselves a new PR :D |
|
Instead of commenting them out can't we use two lists of laws? Then at least io and deferred are well tested and any future changes that might break will be caught |
|
In those cases what we normally do is add the tests directly over their files, i.e. IOTest, DeferredTest, etc. even if they're almost literal duplicates. I'm not sure if this is the best approach, but I'm now sure we want to have @raulraja for second opinion |
|
I think the docs on each one of the wrappers that are not stack-safe should state that, so users are aware of the reasons why it's not. Including proper links to the issues that @pakoito created in their repos. We can also display examples how we can make them stack-safe with |
|
I'd rather have a way to tell the monad-suspend laws to skip the problematic tests so we can override in those data types and potentially emit a warning that they have been skipped because of [links here] |
|
So something like a filter/map function that checks laws by name and either removes them or replaces them with an always true law that contains the warning message? Should I do the docs right here or is that better suited in another issue/pr? I am not too familiar with bindingStackSafe tbh so the latter might make more sense. Also regarding Deferred memoizing its result instead of rerunning: Any ideas? Or should that discussion be moved to a new ticket and the law filtered out similar to stack safe for rx2 and reactor? |
|
@1Jajen1 you can split in PRs as you wish it's more manageable. The filtering in the laws can as simple as a Boolean that asks to skip stack-safety related laws and emits a warn for the given data type so that is known wart
I was arguing with Paco that we can use Free or |
|
Added an option to skip and warnings with the link to the tickets. Only test that fails now is the one that ensures MonadDefer instances don't memoize things for DeferredK. Don't really know what to do with it yet... (Might make sense to skip it as well for now and add a ticket/pr trying to fix it, because that may be some work...) Using bindingStackSafe would only be stacksafe inside that binding right? I mean it'd be fine if well documented that without it its not supported, but a general stack safe integration would be better if it does not mean lots of added complexety or overhead. I'll do docs later on or tomorrow morning. If its not too much I'll keep it in this pr. |
|
Added some documentation about rx2 and reactor not being stacksafe, with links to both tickets. |
| @@ -181,6 +181,49 @@ disposable() | |||
| // Boom! caused by BindingCancellationException | |||
| ``` | |||
|
|
|||
| ### Stack safety | |||
| @@ -148,6 +148,49 @@ disposable() | |||
| // Boom! caused by BindingCancellationException | |||
| ``` | |||
|
|
|||
| ### Stack safety | |||
I'll take a look, I had some "shower" ideas last morning |
|
We'll need to refactor Deferred for that test to pass, I've fixed it bit needs some cleanup. I say disable it for now, merge this PR and we'll make a ticket to work on deferred ASAP. |
If no one creates a ticket the next few minutes I might not have to change the id :)
|
Commented out and linket to #1156 (which will be the next ticket created if no one does anything the next few minutes 🙈) I'll create the ticket and link back to this as well. |
| @@ -53,10 +55,15 @@ class FluxKTest : UnitSpec() { | |||
| } | |||
| } | |||
|
|
|||
| override fun interceptSpec(context: Spec, spec: () -> Unit) { | |||
Monad defer laws portet from cats-sync-laws.
Solves #590
Excluded laws are: (by function name from cats)
Also there seems to be something wrong with DeferredK as it fails the test for delay suspends evaluation. I believe it is because it is using an async instance of DeferredK which overrides delay to invoke
DeferredK.invokewhich does not start a coroutine withCoroutineStart.LAZYbut withCoroutineStart.DEFAULT.