Add a sample that uses a workflow to lock resources #172

nagl-temporal · 2025-04-01T23:51:35Z

What was changed

Adds a new sample that demonstrates how to use a workflow to mediate access to a small pool of resources.

Why?

A customer asked for an example of how to do this kind of thing. We ruled out worker-specific task queues and sessions - those work best when you can run the worker in/near the protected resource. They cannot, for security reasons, which leaves us with:

non-temporal approaches to resource locking, or
something like this

Checklist

How was this tested:
Ran the worker and starter per the readme. New unit test passes.
Any docs updates needed?
No - it's just a sample.

CLAassistant · 2025-04-01T23:51:41Z

All committers have signed the CLA.

CLAassistant · 2025-04-01T23:51:42Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cretz · 2025-04-02T13:57:51Z

There is a pattern for doing this with signal-with-start. You can see a sample in .NET, Go, and TypeScript.

Would totally support such a sample in Python. Needs to be a simple Lock class usable in a workflow with similar semantics to https://docs.python.org/3/library/asyncio-sync.html#asyncio.Lock but does signal with start and such. Also make sure we have a test or two. Feel free to adapt this PR (or close it and I can make an issue tracking that we need such a sample).

nagl-temporal · 2025-04-11T01:06:52Z

resource_locking/resource_allocator.py

+            resource.autorelease = True
+            yield resource
+        finally:
+            if resource.autorelease:


Blah. I want there to be a way to tell whether the workflow code is CAN'ing here, but I believe there isn't one.

I think you should always release it if a user is using async with. It will be very confusing if I as a user use async with and the resource remains when that exits. If we need to let callers keep the resource across continue as new, they should "detach" it (e.g. can offer a helper for this that returns some type) and "reattach" it on next workflow use. Granted I also think including continue as new from the caller side in this sample is a bit confusing, but maybe it's needed.

resource_locking/resource_allocator.py

nagl-temporal · 2025-04-11T01:16:52Z

@cretz LMK what you think about the async with abstraction and how it's rough around the edges re: continue-as-new. I also didn't code .lock() and .unlock()... could do if needed.

nagl-temporal · 2025-04-11T01:17:18Z

resource_locking/resource_allocator.py

+    async def acquire_resource(
+        cls,
+        *,
+        already_acquired_resource: Optional[AcquiredResource] = None,


I don't like this, but... (thought finished after ⭐)

nagl-temporal · 2025-04-11T01:18:53Z

resource_locking/resource_locking_workflow.py

+    @workflow.run
+    async def run(self, input: ResourceLockingWorkflowInput):
+        async with ResourceAllocator.acquire_resource(
+            already_acquired_resource=input.already_acquired_resource


⭐ ...I had to have some way of handling the case where my CAN-predecessor already locked the resource. It was either an optional param to acquire_resource or branching around the async with, which felt much more awkward.

Not sure this sample needs to complication of including continue as new support for this workflow using the resource manager. This workflow isn't large enough to ever need to continue as new, so including support for it is a bit confusing (people aren't going to understand why it is here, but think they need to do the same).

However, if you do keep this, I would call this reattach= and have it accept an typed object that came from detach in the pre-CAN workflow.

cretz

Added a bunch of comments, can take em or leave em, some of which are self-contradictory. Now that I've seen the full use case, I would think of this as a ResourcePool (with a ResourcePoolWorkflow and a ResourcePoolClient)

resource_locking/lock_manager_workflow.py

cretz · 2025-04-11T12:56:34Z

resource_locking/resource_allocator.py

+            start_signal="acquire_resource",
+            start_signal_args=[AcquireRequest(info.workflow_id)],


This would usually be a good opportunity to use our new update-with-start functionality, but if you must span continue-as-new on the lock manager workflow, you have to stay with signals for now

I do think it's important to keep CAN in. Without CAN, the sample can't demonstrate how to detach/reattach so it seems incomplete to me.

resource_locking/resource_allocator.py

cretz · 2025-04-11T13:01:49Z

resource_locking/resource_allocator.py

+            resource.autorelease = True
+            yield resource
+        finally:
+            if resource.autorelease:


I think you should always release it if a user is using async with. It will be very confusing if I as a user use async with and the resource remains when that exits. If we need to let callers keep the resource across continue as new, they should "detach" it (e.g. can offer a helper for this that returns some type) and "reattach" it on next workflow use. Granted I also think including continue as new from the caller side in this sample is a bit confusing, but maybe it's needed.

resource_locking/starter.py

nagl-temporal · 2025-04-11T15:24:39Z

Thank you - I like the new names. I'll take most of the comments. I want to leave CAN in, because that's the only thing that makes this tricky - sample feels incomplete without it. ~~However, if I add .lock() and .unlock() and use those, the awkwardness goes away.~~ reattach seems fairly natural now, under the new names.

cretz

Looks great, only a few minor things

resource_pool/starter.py

resource_pool/resource_pool_client.py

cretz · 2025-04-15T13:10:33Z

resource_pool/resource_pool_client.py

+        await handle.signal(
+            "acquire_resource", AcquireRequest(workflow.info().workflow_id)
+        )


Suggested change

await handle.signal(

"acquire_resource", AcquireRequest(workflow.info().workflow_id)

)

await handle.signal(

ResourcePoolWorkflow.acquire_resource, AcquireRequest(workflow.info().workflow_id)

)

To be more type safe. Same everywhere

Almost everywhere - the pool workflow should signal the requester in an un-typesafe way (since it can't know what kind of workflow the requester is).

resource_pool/resource_pool_client.py

resource_pool/resource_pool_workflow.py

tests/resource_locking/__init__.py

tests/resource_locking/workflow_test.py

nagl-temporal · 2025-04-15T23:48:20Z

resource_pool/resource_pool_workflow.py

+                f"assign_resource_{workflow.info().workflow_id}",
+                AcquireResponse(release_key=release_signal, resource=resource),
+            )
+


There's no race here, right? Imagine the release signal arrives immediately. The fix is obvious if there is a race, but the code smells nicer this way.

Why I think there's no race: I believe that this handler must run to completion before the handler for the release signal can start.

Correct, there is no race here because this is only called in one place, serially, in the primary loop. However, this can show the dangers of overly extracting/modularizing single-use methods - it can be hard to see the constraints they expect of the callers. Can technically add a "Not safe for concurrent use" docstring if concerned.

cretz

Only minor, non-blocking things, LGTM. Merge at will or let me know when done and I can.

resource_pool/resource_pool_workflow.py

cretz · 2025-04-16T13:33:49Z

resource_pool/resource_pool_workflow.py

+                f"assign_resource_{workflow.info().workflow_id}",
+                AcquireResponse(release_key=release_signal, resource=resource),
+            )
+


Correct, there is no race here because this is only called in one place, serially, in the primary loop. However, this can show the dangers of overly extracting/modularizing single-use methods - it can be hard to see the constraints they expect of the callers. Can technically add a "Not safe for concurrent use" docstring if concerned.

resource_pool/pool_client/resource_pool_client.py

nagl-temporal added 13 commits April 10, 2025 14:55

Add a sample resource locking workflow

6fbfdd9

Fix a code snippet in README.md

00c1d4d

Clean up imports, exercise continue_as_new behavior

cd31a61

Rename the workflows for clarity, improve README

7fa82b1

Remove spurious logging comment

583be40

poe format

be1d760

poe lint

ca184ed

Borrow the random signal name trick from other samples

0e0bcd2

Simplify allocation algorithm

90aaf99

Add 'async with' syntactic sugar

72a24b0

Update readme

eb92f48

poe lint/fmt

69751bb

Add a test

2ab9dfa

nagl-temporal force-pushed the nathan/sem-wf branch from c42ea19 to 2ab9dfa Compare April 11, 2025 01:05

nagl-temporal commented Apr 11, 2025

View reviewed changes

resource_locking/resource_allocator.py Outdated Show resolved Hide resolved

nagl-temporal requested a review from cretz April 11, 2025 01:16

nagl-temporal commented Apr 11, 2025

View reviewed changes

method rename

bb9cb64

cretz reviewed Apr 11, 2025

View reviewed changes

nagl-temporal added 4 commits April 11, 2025 09:15

renames

d9a5f50

type hints for all returns

d75d2d3

handle substantive PR feedback

c38f803

clean up imports

f31cd1b

nagl-temporal requested a review from cretz April 11, 2025 19:04

cretz reviewed Apr 15, 2025

View reviewed changes

nagl-temporal added 3 commits April 15, 2025 16:03

PR feedback (no move yet)

d57b65a

Move the resource pool client

bbe3383

Handle assignment signal failures

104b43f

nagl-temporal commented Apr 15, 2025

View reviewed changes

nagl-temporal force-pushed the nathan/sem-wf branch from 44ce942 to 792276c Compare April 15, 2025 23:53

format/lint

7ac42cc

nagl-temporal force-pushed the nathan/sem-wf branch from 792276c to 7ac42cc Compare April 15, 2025 23:58

nagl-temporal requested a review from cretz April 16, 2025 00:07

cretz approved these changes Apr 16, 2025

View reviewed changes

PR feedback (move resource_pool_workflow, misc factoring)

883f159

nagl-temporal merged commit 1b6145a into temporalio:main Apr 16, 2025
11 checks passed

		start_signal="acquire_resource",
		start_signal_args=[AcquireRequest(info.workflow_id)],

Add a sample that uses a workflow to lock resources #172

Add a sample that uses a workflow to lock resources #172

Uh oh!

Conversation

nagl-temporal commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was changed

Why?

Checklist

Uh oh!

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

Uh oh!

Uh oh!

CLAassistant commented Apr 1, 2025

Uh oh!

cretz commented Apr 2, 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!

Uh oh!

nagl-temporal commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nagl-temporal Apr 11, 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!

cretz Apr 11, 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!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nagl-temporal commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cretz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

nagl-temporal commented Apr 1, 2025 •

edited

Loading

CLAassistant commented Apr 1, 2025 •

edited

Loading

cretz commented Apr 2, 2025 •

edited

Loading

nagl-temporal commented Apr 11, 2025 •

edited

Loading

nagl-temporal Apr 11, 2025 •

edited

Loading

cretz Apr 11, 2025 •

edited

Loading

nagl-temporal commented Apr 11, 2025 •

edited

Loading