Allows users to pass in their own Supervisor for async steps #77
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adzz
force-pushed
the
allow_user_supervisor
branch
from
ad5ff33
to
ad51728
Compare
Imagine an app called Blog that uses Sage. Currently Sage allows async steps that spin up a task supervised under `Sage.AsyncTransactionSupervisor`. Because Sage is a dep of Blog, on shutdown Blog will shutdown _before_ Sage does. That means if an async step interacts with a process under Blog's supervision tree and you are using some sort of rolling deploy you can enter the following scenario: 1. Blog starts async task 2. Sage starts doing that task 3. Blog shuts down 4. Sage tries to interact with a process in Blog and can't - so it crashes / errors. Possible Solutions 1. Make Sage a library and allow users to start it themselves? 2. Allow the user to pass in their own supervisor under which it can start the async steps. This PR does 2.
|
That's a pretty good idea. We should let caller specify which task supervisor the task should run under. |
|
Hello @Adzz, I love the change. I think the only things left are tests and adding documentation to functions explaining how we can override the default supervisor. |
|
Great stuff I'll add that now. |
|
@AndrewDryga I have added a test and some docs, please let me know if you have any suggestions. |
Adzz
force-pushed
the
allow_user_supervisor
branch
from
ad5dcf6
to
ad5ca6d
Compare
AndrewDryga
reviewed
Sep 20, 2022
test/sage/executor_test.exs
Outdated
|
|
||
| # The async task will be awaited as it's the last step in the pipeline meaning | ||
| # there's no chance that we reach here _before_ the send has happened (causing a flaky test) | ||
| assert_received({^ref, ancestors}, 1_000) |
There was a problem hiding this comment.
I would keep the default timeout here, if it starts to fail and become flaky, that's either a reason to increase it globally on CI (because of slow VMs) or there is something worth investigating.
I would also remove the comment above, I think await behavior is already explained in docs.
AndrewDryga
reviewed
Sep 20, 2022
Co-authored-by: Andrew Dryga <andrew@dryga.com>
AndrewDryga
reviewed
Sep 20, 2022
Comment on lines
+392
to
+395
| * `:supervisor` - the name of a supervisor that the task will be spawned under. Defaults to the\ | ||
| `Sage.AsyncTransactionSupervisor` started by Sage (meaning it will exist under Sage's supervision\ | ||
| tree). It might be a good idea to pass your own in to avoid race conditions on shutdown if your\ | ||
| step interacts with a process in your own app's supervision tree. |
There was a problem hiding this comment.
Suggested change
| * `:supervisor` - the name of a supervisor that the task will be spawned under. Defaults to the\ | |
| `Sage.AsyncTransactionSupervisor` started by Sage (meaning it will exist under Sage's supervision\ | |
| tree). It might be a good idea to pass your own in to avoid race conditions on shutdown if your\ | |
| step interacts with a process in your own app's supervision tree. | |
| * `:supervisor` - the name of a `Task.Supervisor` that the task will be spawned under. Defaults to the\ | |
| `Sage.AsyncTransactionSupervisor` started by Sage (meaning it will exist under Sage's supervision\ | |
| tree). It might be a good idea to pass your own in to avoid race conditions on shutdown if your\ | |
| step interacts with a process in your own app's supervision tree. |
|
@Adzz don't worry about docs formatting, LGTM. I'll slightly edit them and release a new version. Thank you ❤️ |
|
Sounds good thanks for the quick feedback! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Imagine an app called Blog that uses Sage.
Currently Sage allows async steps that spin up a task supervised under
Sage.AsyncTransactionSupervisor. Because Sage is a dep of Blog, on shutdown Blog will shutdown before Sage does.That means if an async step interacts with a process under Blog's supervision tree and you are using some sort of rolling deploy you can enter the following scenario:
Possible Solutions
This PR does 2.