Repository navigation
Conversation
Adds an `abortWithParent` option to `Actor.start`, `call`, `startTask` and `callTask`. It is recorded with the named child run, and on the `aborting` event every child run started with it is gracefully aborted. Closes #739
github-actions
Bot
added
the
t-tooling
apify-service-account
added
the
tested
|
✅ The link checker did not find any broken links. See more at https://github.com/apify/apify-sdk-js/actions/runs/39428850912#summary-118648267280 |
vdusek
left a comment
vdusek
left a comment
There was a problem hiding this comment.
One comment from me, two from Claude.
I'd like mine to be resolved before we merge either the JS or the Python PR.
| const events = serviceLocator.getEventManager(); | ||
| // Removing all listeners keeps the SDK's own one that aborts the child runs started with `abortWithParent`. | ||
| const keepsChildRunAbort = !listener && events.listeners(event).includes(this.#abortChildRunsWithParent); | ||
|
|
||
| events.off(event, listener); | ||
|
|
||
| if (keepsChildRunAbort) events.on(event, this.#abortChildRunsWithParent); |
There was a problem hiding this comment.
TLDR; how to approach "internal" listeners?
This only protects the listener when removal goes through Actor.off(). A direct serviceLocator.getEventManager().off('aborting') still drops it, and the listener also shows up in events.listeners('aborting').
Python keeps SDK-internal listeners in a separate list on the event manager and dispatches them from an emit() override, so no off() call can reach them. But maybe there is a better way? 🙂
| if (this.#gracefulShutdownHandlers.migrating) { | ||
| this.off(ACTOR_EVENT_NAMES.MIGRATING, this.#gracefulShutdownHandlers.migrating); | ||
| } | ||
| this.off(ACTOR_EVENT_NAMES.ABORTING, this.#abortChildRunsWithParent); |
There was a problem hiding this comment.
Note: The listener is removed before events.close() closes the websocket, so an aborting event that arrives during exit() aborts no child runs. Python removes it only after the event manager is closed. Moving this line below await events.close() would match.
✍️ Drafted by Claude Code
| * Gracefully aborts the current run under each name tracked with `abortWithParent`, and makes any later start with | ||
| * `abortWithParent` throw. A start in flight is waited for, so the run it starts is aborted too. | ||
| */ | ||
| async abortRunsWithParent(defaultClient: ApifyClient) { |
There was a problem hiding this comment.
Suggestion: exit() waits for this listener inside events.close(), which has no timeout, and the process.exit fallback timer is only set after that. A slow abort request (client retries) can hold up exit. A timeout around the aborts would bound it.
✍️ Drafted by Claude Code
The listener that aborts the child runs started with `abortWithParent` is now an internal listener of `PlatformEventManager`, which `off()` doesn't remove and `listeners()` doesn't return, whether called through `Actor` or on the event manager directly. `exit()` removes it only after the events are closed, so an `aborting` event that arrives meanwhile still aborts the child runs.
`exit()` waits for the `aborting` listeners while closing the events, with no timeout of its own and before the `process.exit()` fallback is set, so a slow abort request could hold up the exit past the 30 seconds the platform gives a gracefully aborted run. The aborts are now waited for 10 seconds at most, and the ones still in progress go on in the background.
Documents that a run started with a custom `token` before a migration is aborted with the default client, tests that the abort uses the client the run was started with, and drops a re-throw for an error that can't reach the timeout's catch.
…ger constructor Replaces the public `onInternal()` / `offInternal()` methods, which let any code register or remove listeners that `off()` can't reach. The Actor now passes its listener that aborts the child runs when it creates its event manager, and it stays there for the event manager's lifetime, so `init()` and `exit()` no longer register or remove it.
Adds an
abortWithParentoption toActor.start,call,startTaskandcallTaskthat is recorded with the named child run, so the run is gracefully aborted on the parent'sabortingevent, also after a migration.Closes #739