search

rock-core / tools-roby

The roby plan manager
Other
3 stars 11 forks source link

fix(core): propagate errors that come from a promise's own error handlers #201

Closed doudou closed 3 years ago

doudou commented 3 years ago

Roby's Promise class allows to handle errors with #on_error. Having error handlers defined would cause ExecutionEngine's promise "watch" method to ignore the failed promise and remove it.

It would not account for an error handler that itself would raise. This should not happen, but it obviously will happen (I should say "did"), and ignoring that error is a really bad thing.

Propagate the error as a framework error.

doudou commented 3 years ago

Ping ? (Not complaining at all .. thanks a lot for the awful lot of reviewing from the last two weeks)

g-arjones commented 3 years ago

Sorry, I forgot to submit the review.

thanks a lot

No problem. Btw, I really appreciate the improvements on iodrivers_base and RTT... :+1:

doudou commented 3 years ago

I really appreciate the improvements on iodrivers_base and RTT... +1

Not Syskit ... tssss ;-) I'm pretty happy on the jump in robustness the last two weeks of work probably gave us.

Does it still make sense for you to review the Syskit/Roby pull requests, btw ? Given the total absence of questions about those, I'm guessing you stopped using them a while ago.

g-arjones commented 3 years ago

Does it still make sense for you to review the Syskit/Roby pull requests, btw ?

I do like/enjoy reviewing (even though it's true that I'm not using them that much these days) because (1) I often learn something by reading your code and (2) it's a way of keeping up-to-date with Rock's development. That said, it's hard to provide meaningful input beyond spotting obvious/clear mistakes (which you rarely make) and potential API issues so don't expect too much from me :)

I also still have a CI running with our stuff so I can tell you if something breaks.

doudou commented 3 years ago

:+1: thanks again.

doudou commented 3 years ago

This was actually part of #202 ... Didn't realize it. Just rebased and pushed and github figured it out.