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

EventEngine: fix callers of Run() and RunAfter() to create ExecCtx #31047

Merged
merged 1 commit into from
Sep 20, 2022

Conversation

markdroth
Copy link
Member

See discussion in #31041.

Given the number of places we missed this, I think we may want to consider adding some sort of test to ensure that all callers of Run() or RunAt() will create their own ExecCtx.

CC @ctiller @veblush

@drfloob
Copy link
Member

drfloob commented Sep 19, 2022

Interesting. I was hoping that any missing, necessary exec_ctx instances would be ... less subtle than this. We had discussed trying to avoid a blanket policy to add them to every callback. I'm not sure they're needed in all of these cases, but we aren't paying a huge price to add them, so I'm ok with it.

@markdroth
Copy link
Member Author

I don't think we can assume that the ExecCtx is not needed in any case that calls EventEngine::Run() or EventEngine::RunAt(). There are too many subtle ways that callback ordering can cause bugs.

I don't think we can start removing ExecCtx instances until we remove all the code that actually uses the ExecCtx.

@veblush veblush merged commit 5e0165b into grpc:master Sep 20, 2022
@markdroth markdroth deleted the ee_exec_ctx_fix branch September 20, 2022 16:16
veblush added a commit that referenced this pull request Sep 20, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/c++ lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants