add sleep-for-days sample by THardy98 · Pull Request #95 · temporalio/samples-dotnet

THardy98 · 2025-02-01T06:22:29Z

added sleep-for-days sample

THardy98 · 2025-02-01T06:24:20Z

This is pretty much done - I'm just having trouble with the:
await env.DelayAsync(TimeSpan.FromDays(90));

it's not actually time-skipping and I'm not sure why given that it's essentially following what's in the docs

I've made the assertion following that statement to fail, just to make sure that this doesn't pass CI.

cretz · 2025-02-03T16:08:01Z

src/SleepForDays/Activities.cs

Class name should match file name

Renamed class to Activities

cretz · 2025-02-03T16:08:31Z

src/SleepForDays/SleepForDays.workflow.cs

+using Temporalio.Workflows;
+
+[Workflow]
+public class SleepForDaysWorkflow


Class name should match pre-extension part of file name (should probably change file name in this case)

Renamed file to SleepForDaysWorkflow.workflow.cs, looks a bit odd.

Yeah, though that same seemingly-odd name is present on similar files/workflows in this repo

cretz · 2025-02-03T16:14:42Z

src/SleepForDays/SleepForDays.workflow.cs

Suggested change

isComplete = false;

This is dangerous because it will overwrite a signal that comes before the workflow starts (and bool fields default to false anyways)

Good point, didn't consider this. Don't think this is a real concern with this sample, but I get that this might not be a pattern we want to demonstrate to new users. Removed.

cretz · 2025-02-03T16:14:54Z

src/SleepForDays/SleepForDays.workflow.cs

Suggested change

private bool isComplete;

private bool complete;

"is" prefix not usually needed in .NET

Removed prefix.

cretz · 2025-02-03T16:15:21Z

src/SleepForDays/SleepForDays.workflow.cs

Can be a bit confusing to users for the send-email subject to be what the workflow is doing next, but not a big deal

cretz · 2025-02-03T16:16:03Z

src/SleepForDays/SleepForDays.workflow.cs

There is an overload of WaitConditionAsync that accepts a timeout

We considered this, but figured having a direct sleep more clearly showed the ability to wait for long periods of time.

The concern here is that we usually encourage samples to look like a user should write them. It also depends on if we're making a simple sample like for the website or not. If so and we want a clear "sleep" for this sample, we should consider using workflow cancellation as the interrupt mechanism because, even though we may not recommend cancellation as control flow, it makes the sample much more legible to a first-time viewer.

Yeah that's true. The intention is for this to be a reference for the web sample and the discussion on Slack leaned towards using the workflow function and stripping out the signal stuff if it's too complicated.

But imo I agree. I err on the side of no interrupt mechanism at all, would be the simplest to read for a new viewer. But we have some consensus on this already and some samples already merged. I don't think it's worth re-opening the discussion or being inconsistent with the other samples, especially given the scope.

Won't hold up this PR on this FWIW, but when we get to the public site content, I think it may not look like a sample

Yeah I think that's fair. Marketing team asked for PRs yesterday which I shared, with a little addendum that if they want simpler examples I can send them code snippets with the signal handling stuff removed. I don't think it's a bad thing that the actual sample has the signal handling, it's useful.

cretz · 2025-02-03T16:16:34Z

src/SleepForDays/SleepForDays.workflow.cs

Suggested change

public async Task CompleteAsync()

{

isComplete = true;

}

public async Task CompleteAsync() => isComplete = true;

Can shorten some of these one-liners if you want

cretz · 2025-02-03T16:18:44Z

tests/SleepForDays/SleepForDaysWorkflowTests.cs

+            // Sleep for 90 days
+            await env.DelayAsync(TimeSpan.FromDays(90));


Are you sure you're not time skipping before the workflow is even waiting for the number of days? You may be time skipping before the workflow even reaches the wait condition.

I hadn't considered that. Is there a way to test for this? Maybe with a query?

Yeah, it's a bit rough, but that's what we've done in other situations is waited for a workflow to reach a certain state. One way is to have the activity you're mocking let you know at least that part has been reached. But otherwise, yeah a loop until history has a timer event may be best here.

Added a check that the timer exists before time-skipping. That fixed the time-skipping issue I mentioned at the beginning, seems like that was indeed the cause of the issue. Thanks!

cretz · 2025-02-03T19:27:34Z

src/SleepForDays/SleepForDays.workflow.cs

Suggested change

await Task.WhenAny(

await Workflow.WhenAnyAsync(

Even though Task.WhenAny( technically works here in most cases, we encourage the safer wrapper

Didn't realize there was a workflow wrapper, switched.

Make sure it's WhenAnyAsync

cretz

Also, make sure that this .csproj is added to the primary solution (sln) file. Can see what's there with dotnet sln list and can add with dotnet sln add. Also don't forget to add this sample to README list.

cretz · 2025-02-03T21:11:33Z

tests/SleepForDays/SleepForDaysWorkflowTests.cs

This is failing compilation

Forgot to push that change, whoops.

Added to sln via dotnet sln add src/SleepForDays, seems to look right.
Added to README as well

THardy98 · 2025-02-04T16:49:47Z

Am I correct in thinking that [TimeSkippingServerFact] is used to avoid time skipping on platforms outside of x86/x64?

[xUnit.net 00:00:01.25]       Time-skipping test server only works on x86/x64 platforms

Saw this failure in CI when I had the [Fact] annotation and noticed the [TimeSkippingServerFact] annotation used in ActivitySimple's workflow test.

cretz · 2025-02-04T19:06:11Z

tests/SleepForDays/SleepForDaysWorkflowTests.cs

The final assert can never be false. Would recommend only looping a certain number of times and sleeping a short bit between each fetch.

Actually, I just realized even in samples we have a AssertMore.EventuallyAsync helper, so you could probably just have:

await AssertMore.EventuallyAsync(async () => { var history = await handle.FetchHistoryAsync(); Assert.Contains(history.Events, e => e.TimerStartedEventAttributes != null); });

(untested but the idea is there)

Oh much nicer. I've also updated the activity execution count assertions. By the time the timer event for the sleep/delay is in history, we should have already executed the activity once. Skipping 90 more days should then give us 4 total executions not 3, which was erroneous. I wonder why that passed, if the timer existed in history before, then we still would have had to execute the activity, we still should have had 4 executions not 3.

cretz · 2025-02-04T19:25:29Z

Am I correct in thinking that [TimeSkippingServerFact] is used to avoid time skipping on platforms outside of x86/x64?

Yes, though we should probably relax it one day for macOS since its translation layer can run x64 binaries

cretz

Looks great, feel free to merge if/when CI passes

THardy98 · 2025-02-05T02:13:05Z

TYFR!

add sleep-for-days sample

25059a8

THardy98 requested a review from cretz February 1, 2025 06:22

cretz reviewed Feb 3, 2025

View reviewed changes

THardy98 added 2 commits February 3, 2025 11:54

address pr comments

5ff4101

WhenAny -> WhenAnyAsync

36b1cc8

cretz reviewed Feb 3, 2025

View reviewed changes

fixed test - added check for sleep timer start before time-skipping

761237d

THardy98 requested a review from cretz February 4, 2025 16:43

THardy98 added 2 commits February 4, 2025 08:44

ran dotnet format

e3619e7

changed [Fact] -> [TimeSkippingServerFact]

506a7b1

cretz reviewed Feb 4, 2025

View reviewed changes

use to check for eventuality of timer history event

6fd039f

cretz approved these changes Feb 4, 2025

View reviewed changes

THardy98 merged commit 98bb677 into main Feb 5, 2025
7 checks passed

cretz deleted the sleep-for-days branch February 5, 2025 14:48

-    public async Task CompleteAsync()
-    {
-        isComplete = true;
-    }
+    public async Task CompleteAsync() => isComplete = true;

		// Sleep for 90 days
		await env.DelayAsync(TimeSpan.FromDays(90));

Conversation

THardy98 commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

THardy98 commented Feb 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 commented Feb 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

THardy98 commented Feb 1, 2025 •

edited

Loading

THardy98 commented Feb 1, 2025 •

edited

Loading

cretz Feb 3, 2025 •

edited

Loading

cretz Feb 3, 2025 •

edited

Loading

THardy98 Feb 4, 2025 •

edited

Loading

THardy98 Feb 4, 2025 •

edited

Loading

THardy98 Feb 4, 2025 •

edited

Loading

cretz commented Feb 4, 2025 •

edited

Loading