Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Log and drop signals whose params can't be deserialized (and other error handling improvements) #349

Merged
merged 4 commits into from
Jul 18, 2023

Conversation

cretz
Copy link
Member

@cretz cretz commented Jul 17, 2023

What was changed

  • Log and drop signals whose parameters cannot be deserialized. This should not be a breaking change for anyone as the behavior before was to fail the task
  • Support type hint helper properly ignoring self with new 3.11 typing.Self hint
  • Start catching BaseException in activities. There should not be a reason (even a GeneratorExit or KeyboardInterrupt) where we wouldn't want to catch it

Checklist

  1. Closes [Bug] Signals that can't be deserialized should be dropped/ignored #347
  2. Closes [Bug] If self parameter is type hinted, it is not properly ignored when deserializing parameters #318
  3. Closes [Bug] BaseException exceptions are not caught when raised in an activity #333

@cretz cretz requested a review from a team as a code owner July 17, 2023 18:09
defn.arg_types,
defn.dynamic_vararg,
)
except Exception:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the only things that could get raised from the call are decoding problems... but maybe it'd be a good protection to catch something a little more specific here, if that can be done w/o any incompatible change.

Copy link
Member Author

@cretz cretz Jul 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want the change technically incompatible. This would fail the task before and I think the requirement is "any deserialization failure during signals causes log-and-drop". There is nothing more specific I can think of here. I guess I could skip Temporal failures, but I don't think we want to let any deserialization exceptions escape. If we do, I think I'd want to make it an option (failing the workflow from the signal arg converter is a bit rough, but may be wanted).

@cretz cretz merged commit b9df212 into temporalio:main Jul 18, 2023
12 checks passed
@cretz cretz deleted the error-handling-improvements branch July 18, 2023 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants