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

Moving RuntimeContext check inside ReminderRegistry #8436

Merged

Conversation

cmeyertons
Copy link
Contributor

@cmeyertons cmeyertons commented May 15, 2023

Motivation

In unit testing libraries (such as OrleansTestKit) the RuntimeContext check for reminder-based grain calls proves very challenging.

Because RuntimeContext uses a thread-local mechanism, it becomes non-trivial to ensure the unit test runs on the same thread as the calling code (imagine some IO-bound async behavior happening between the unit test & the Reminder call)

This is related to some of the conversation going on in OrleansContrib/OrleansTestKit#134

Also related to #8387 - RuntimeContext hard to test

Proposed Fix

Move the RuntimeContext check into the ReminderRegistry so that it sits behind a stubbable testing seam interface instead of residing in an extension method. This fix should have very similar performance as the prior code.

This change also provides more consistent w/ the Timer Registry implementation, which defers this check to inside the GrainTimer code

Alternatives considered:

  1. Extending ReminderOptions to control if the RuntimeContext check occurs
  2. Implementing a singleton IReminderRegistryResolver that provides a testing seam to control this logic.

cc @ReubenBond @seniorquico

Microsoft Reviewers: Open in CodeFlow

@ReubenBond
Copy link
Member

Beautiful, thank you!

@ReubenBond ReubenBond merged commit 7243af9 into dotnet:main May 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants