fix(statemachine): do not raise from state.run #1115
Conversation
- re-raise CancelledError - don't return State on CatchableError - expect the Proofs_InvalidProof custom error instead of checking a string
This was swallowing a KeyError in one of the tests (fixed in the previous commit)
There was a problem hiding this comment.
Did you encounter some problem related to this? Not fan of the verbosity, but I guess overall is an improvement 😅
Just to double-check, I see lots of swallowing of CancelledErrors - that is because the states runs are asyncSpawned right?
An idea - maybe as to elevate some of the added verbosity, we could allow CancelledError in the raises and it could be left to propagate to the Machine.scheduler() where it is already now discarded anyhow?
| ended.cancelSoon() | ||
| return some State(PurchaseFailed()) | ||
| else: | ||
| failed.cancelSoon() |
There was a problem hiding this comment.
Shouldn't these be then cancelled in the CatchableError branch as well?
There was a problem hiding this comment.
It's a good point and I honestly don't know the answer. My initial assumption was that once the reference to the run proc was GC'd (during an exception), that the futures (ended and failed) would be GC'd, but i spent a bit of time looking at the values in the heap (using -d:nimTypeNames and dumpNumberOfInstances()) and honestly I can't tell if those futures are GC'd or not. So I've asked Eugene: https://discord.com/channels/613988663034118151/616299964242460682/1341328004923658251
There was a problem hiding this comment.
Nice, thanks @emizzle! It is indeed much nicer to let states handle their own errors.
codex/sales/states/proving.nim
Outdated
|
|
||
| return some State(SalePayout()) | ||
| except CancelledError as e: | ||
| trace "SalePreparing.run onCleanUp was cancelled", error = e.msgDetail |
There was a problem hiding this comment.
| trace "SalePreparing.run onCleanUp was cancelled", error = e.msgDetail | |
| trace "SaleProving.run onCleanup was cancelled", error = e.msgDetail |
Same below
There was a problem hiding this comment.
Yikes, sorry mark, I missed this one. IMO, onCleanup should be removed entirely from that trace log. Can do in a future PR.
Yes, overall, I have come back to fixing this at least 3 times because I've spent so much time debugging it. When there's CancelledErrors leaking out, it becomes very difficult to reason about what is happening from the log output, especially when they're propagating the stack invisibly. Changing to
Yes, that's correct. If they are not swallowed, the
Yes, that's a good idea, which as you said is how we have it now, but in my mind, it's easier to reason about |
* fix(statemachine): do not raise from state.run * fix rebase * fix exception handling in SaleProvingSimulated.prove - re-raise CancelledError - don't return State on CatchableError - expect the Proofs_InvalidProof custom error instead of checking a string * asyncSpawn salesagent.onCancelled This was swallowing a KeyError in one of the tests (fixed in the previous commit) * remove error handling states in asyncstatemachine * revert unneeded changes * formatting * PR feedback, logging updates
Prevents the state machine's run function from raising exceptions, ensuring there are no exceptions leaking out, and that each state can handle exceptions on an individual basis, instead of handling them all the same at the statemachine level.