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

Block mutating workflow state in a read only context #1821

Merged

Conversation

Quinn-With-Two-Ns
Copy link
Contributor

Block calling most mutating workflow functions in read only context. This includes update validators and side effects.

Note: This does not include queries because queries are not run in a workflow thread

@Quinn-With-Two-Ns Quinn-With-Two-Ns marked this pull request as ready for review July 20, 2023 16:24
@Quinn-With-Two-Ns Quinn-With-Two-Ns requested a review from a team as a code owner July 20, 2023 16:24
@Sushisource
Copy link
Member

Hmm, the way I imagined in my head this would work is rather than passing this assertReadOnly callback into all the different handlers, you'd instead just wrap the act of inserting into the command buffer, and complain if you're in a readonly context - that way you don't have to pass that callback all over.

Also, for setting/removing the readonly flag, maybe some RAII pattern would be safer than try/finally?

Do either/both of those feel reasonable to you?

@Quinn-With-Two-Ns
Copy link
Contributor Author

Hmm, the way I imagined in my head this would work is rather than passing this assertReadOnly callback into all the different handlers, you'd instead just wrap the act of inserting into the command buffer, and complain if you're in a readonly context - that way you don't have to pass that callback all over.

No, by the point your going into the command buffer is too late, we want to fail before you even go through interceptors. This is how it works in Python and Go. Also some illegal calls do not insert anything into the command buffer or only conditionally do so It makes the code more confusing and harder to verify correctness IMO since you'd have checks at very different level.

Also, for setting/removing the readonly flag, maybe some RAII pattern would be safer than try/finally?

Java doesn't really have RAII, the only other thing we could do it a try-with-resources. I don't think it would improve the safety much since you just change what you have to remember from "remember to set back to false" to "remember to use this wrapper object"

@Sushisource
Copy link
Member

Sushisource commented Jul 21, 2023

OK - at the end of the day you're failing the WFT still though, so does it really matter at what point you do that? I get the bit about some things not generating commands though.

And yeah, try-with is the type of thing I'm talking about. I think ""remember to set back to false" to "remember to use this wrapper object"" is an improvement. It's remember one thing instead of two things.

@Quinn-With-Two-Ns Quinn-With-Two-Ns merged commit 16755a1 into temporalio:master Jul 21, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants