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

Make pause and resume deadlock detector functions public #1647

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

itayd
Copy link

@itayd itayd commented Sep 24, 2024

What was changed

Expose PauseDeadlockDetector and ResumeDeadlockDetector publicly.

Why?

See temporalio/temporal#6546:

We need to run some function that might block for a while (up to 10s, usually) during a Temporal workflow. That function cannot receive a Temporal workflow context. It also has to run every time the workflow runs, including replays. Further operations during this workflow rely on this function being run first.

Currently when we do this, the deadlock detection mechanism panics.

Checklist

  1. Closes #6546.

@itayd itayd requested a review from a team as a code owner September 24, 2024 03:44
@CLAassistant
Copy link

CLAassistant commented Sep 24, 2024

CLA assistant check
All committers have signed the CLA.

@itayd itayd changed the title make pause and resume deadlock detector functions public Make pause and resume deadlock detector functions public Sep 24, 2024
@Quinn-With-Two-Ns
Copy link
Contributor

Quinn-With-Two-Ns commented Sep 24, 2024

Pausing and unpausing the deadlock detector is a very dangerous API, I do not think we should expose this to users as the potential for misuse far outweigh the legitimate reasons to use this. I would stick with the DataConverterWithoutDeadlockDetection approach. If you can describe your use case in more detail perhaps there is an interceptor you can use (or we can add) because this is not a sustainable approach to the problem

@itayd
Copy link
Author

itayd commented Sep 24, 2024

... I do not think we should expose this to users ...

It is now already practically exposed to users in a very ugly way, which might make it hard to find (ab)users if you wish to change how the converter works.

See also the related issue - if just calling it this way is not palatable, maybe we should try to do something such as:

func WithoutDeadlockDetector(f func())

@itayd
Copy link
Author

itayd commented Sep 24, 2024

If you can describe your use case in more detail perhaps there is an interceptor you can use (or we can add) because this is not a sustainable approach to the problem

Described in issue, but I'd be happy to elaborate more if needed.

What do you mean by "interceptor"? Do you have any examples?

@Quinn-With-Two-Ns
Copy link
Contributor

We do not want to make it easier for users to disable deadlock detection. Disabling the deadlock detector in arbitrary sections of code should be difficult by design.

Described in issue, but I'd be happy to elaborate more if needed.

Yes please, it is not clear what you are trying to do and where your requirements come from

What do you mean by "interceptor"? Do you have any examples?

All SDKs have interceptors users can register when creating workers/clients that certain important calls will go through. You would probably be interested in the WorkflowInboundInterceptor Init here is called before the main workflow function is run outside of the deadlock detector so it may be viable for your use case.

we have a sample showing how to create your own here

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.

3 participants