fix(core): Use AbortController to notify nodes to abort execution #6141

netroy · 2023-04-28T15:03:42Z

and add support for cancelling ongoing operations inside a node.

OlegIvaniv · 2023-11-15T10:35:21Z

@netroy I've been seeing a lot of MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit errors lately with these changes, any idea what might be wrong?

netroy · 2023-11-15T10:56:48Z

@netroy I've been seeing a lot of MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit errors lately with these changes, any idea what might be wrong?

I'm guessing that this is coming from the fact that every node you pass down the AbortSignal into is going to add a listener on it. And since a workflow could have dozens (if not hundreds) of nodes using this mechanism, we need to do a (abortSignal as unknown as EventEmitter).setMaxListeners(Infinity); when the AbortController is created.

I'll fix that in this PR. but, since this is just a warning, I assume that your aren't blocked by this.

OlegIvaniv · 2023-11-15T12:42:36Z

@netroy I've been seeing a lot of MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit errors lately with these changes, any idea what might be wrong?

I'm guessing that this is coming from the fact that every node you pass down the AbortSignal into is going to add a listener on it. And since a workflow could have dozens (if not hundreds) of nodes using this mechanism, we need to do a (abortSignal as unknown as EventEmitter).setMaxListeners(Infinity); when the AbortController is created.

I'll fix that in this PR. but, since this is just a warning, I assume that your aren't blocked by this.

Ah, understood. No, not blocked, was just worried it might have been something more nefarious. Sounds like we're all good then. Appreciate the context.

… seconds)

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

…ListenersExceededWarning

netroy · 2023-11-24T11:50:41Z

packages/core/src/WorkflowExecute.ts

@@ -830,11 +820,16 @@ export class WorkflowExecute {
 		let closeFunction: Promise<void> | undefined;

 		return new PCancelable(async (resolve, reject, onCancel) => {


once this PR is merged, I'm going to get rid of PCancelable and use a regular promise with AbortController instead.
Even p-cancelable recommends to do that since 3 years.
Moving away from PCancelable should also make this code more readable.

packages/cli/src/databases/repositories/execution.repository.ts

ivov · 2023-11-24T14:38:31Z

packages/core/src/WorkflowExecute.ts

+				this.abortController.abort();
+				const fullRunData = this.getFullRunData(startedAt);
+				void this.executeHook('workflowExecuteAfter', [fullRunData]);
+				setTimeout(() => resolve(fullRunData), 10);


For understanding - We are resolving this PCancelable inside a timeout in order to allow for other promises to resolve first, i.e. to force this promise to be placed in the macrotask queue, processed after the microtask queue. Is this correct? Else why are we doing this?

yeah. we have a bunch of async operations (that don't really take any time, and possibly don't even need to be async) that need to happen between the cancellation, and before this resolve is called. I don't like this, and plan to fix this in the same PR where I'll remove PCancelable completely.

packages/cli/src/ActiveExecutions.ts

ivov · 2023-11-24T15:02:03Z

packages/core/src/WorkflowExecute.ts


 			onCancel.shouldReject = false;
 			onCancel(() => {
-				gotCancel = true;
+				this.status = 'canceled';
+				this.abortController.abort();


Looks like we can pass in a reason arg - all cancellations that can happen right now are user-requested? Else we might want to use this arg to differentiate between user-requested cancellations vs. cancellations initiated on our end?

Good idea. we can have cancellation from user interaction, timeout, or server shutdown.
I've created a backlog ticket: ENG-92.

ivov · 2023-11-24T15:05:13Z

packages/editor-ui/src/mixins/pushConnection.ts

 					runDataExecutedErrorMessage = this.$locale.baseText(
 						'pushConnection.executionFailed.message',
 					);
+				} else if (runDataExecuted.status === 'canceled') {
+					runDataExecutedErrorMessage = this.$locale.baseText(
+						'executionsList.showMessage.stopExecution.message',


We're using canceled internally but stopped externally - we might want to unify for consistency.

Agreed that we should unify these. Created a ticket ENG-93

ivov · 2023-11-24T15:06:04Z

packages/core/src/NodeExecuteFunctions.ts

+		const fn = () => {
+			abortSignal.removeEventListener('abort', fn);
+			handler();
+		};


Would it be useful to log here that a cancellation occurred?

Do you mean logging for every node being aborted? or do you mean we should log when an execution is aborted.
This code is called for individual node that wishes to do something on execution cancellation.

Exactly, I meant for every node aborted - but on second thought, it might be enough to log when the execution itself is cancelled.

for that to work, we'd have to add the log to onCancel, while all the other execution finishing logs are in processSuccessExecution. After PCancelable is gone, this will be less mess, and I'll add this logging then.

packages/core/src/NodeExecuteFunctions.ts

packages/cli/src/services/dynamicNodeParameters.service.ts

…-mode

ivov

Thanks for explaining and addressing everything!

github-actions · 2023-11-24T16:07:47Z

⚠️ Some Cypress E2E specs are failing, please fix them before merging

cypress · 2023-11-24T16:07:54Z

1 flaky test on run #3018 ↗︎

0	283	5	0	1

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 netroy 🗃️ e2e/*
Project: n8n	Commit: `3e4a64672c`
Status: Passed	Duration: 06:12 💡
Started: Nov 24, 2023 5:00 PM	Ended: Nov 24, 2023 5:07 PM

cypress/e2e/12-canvas.cy.ts • 1 flaky test

View Output Video

Test		Artifacts
Canvas Node Manipulation and Navigation > should add merge node and test connections		`Screenshots` `Video`

Review all test suite changes for PR #6141 ↗︎

github-actions · 2023-11-24T17:07:10Z

✅ All Cypress E2E specs passed

# [1.19.0](https://github.com/n8n-io/n8n/compare/n8n@1.18.0...n8n@1.19.0) (2023-11-29) ### Bug Fixes * **core:** Ensure member and admin cannot be promoted to owner ([#7830](#7830)) ([9b87a59](9b87a59)), closes [/linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint#comment-62355f6](https://github.com//linear.app/n8n/issue/PAY-985/add-user-role-modification-endpoint/issues/comment-62355f6) * **core:** Prevent error messages due to statistics about data loading ([#7824](#7824)) ([847f6ac](847f6ac)) * **core:** Tighten checks for multi-main setup usage ([#7788](#7788)) ([fdb2c18](fdb2c18)) * **core:** Use AbortController to notify nodes to abort execution ([#6141](#6141)) ([d2c18c5](d2c18c5)) * **editor:** Add telemetry to workflow history ([#7811](#7811)) ([d497041](d497041)) * **editor:** Allow owners and admins to share workflows and credentials they don't own ([#7833](#7833)) ([3ab3ec9](3ab3ec9)) * **editor:** Disable context menu actions in read-only mode ([#7789](#7789)) ([902beff](902beff)) * **editor:** Fix cloud plan data loading on instance ([#7841](#7841)) ([8b99384](8b99384)) * **editor:** Fix credential icon for old node type version ([#7843](#7843)) ([4074107](4074107)) * **editor:** Fix icon for unknown node type ([#7842](#7842)) ([28ac5a7](28ac5a7)) * **editor:** Fix mouse position in workflow previews ([#7853](#7853)) ([c063398](c063398)) * **editor:** Show nice error when environment is not set up ([#7778](#7778)) ([5835e05](5835e05)) * **editor:** Suppress dev server websocket messages in workflow view ([#7808](#7808)) ([685ffd7](685ffd7)) * **Google Sheets Node:** Read operation execute for each item ([#7800](#7800)) ([d548872](d548872)) * **HTTP Request Node:** Enable expressions for binary input data fields ([#7782](#7782)) ([6208af0](6208af0)) * **Microsoft SQL Node:** Prevent double escaping table name ([#7801](#7801)) ([73ec753](73ec753)) ### Features * Add AI tool building capabilities ([#7336](#7336)) ([87def60](87def60)) * Add initial scope checks via decorators ([#7737](#7737)) ([a37f1cb](a37f1cb)) * Ado 1296 spike credential setup in templates ([#7786](#7786)) ([aae45b0](aae45b0)) * **core:** Add Support for custom CORS origins for webhooks ([#7455](#7455)) ([99a9ea4](99a9ea4)) * **core:** Allow user role modification ([#7797](#7797)) ([7a86d36](7a86d36)) * **core:** Set up endpoint for all existing roles with license flag ([#7834](#7834)) ([2356fb0](2356fb0)) * **editor:** Add node name and version to NDV node settings ([#7731](#7731)) ([da85198](da85198)) * **editor:** Add routing middleware, permission checks, RBAC store, RBAC component ([#7702](#7702)) ([67a8891](67a8891)) * **editor:** Replace middleware for Role checks with Scope checks ([#7847](#7847)) ([72852a6](72852a6)) * **editor:** Show avatars for users currently working on the same workflow ([#7763](#7763)) ([77bc8ec](77bc8ec)) * **Notion Node:** Option to simplify output in getChildBlocks operation ([#7791](#7791)) ([d667bca](d667bca)) * **Slack Node:** Add support for getting the profile of a user ([#7829](#7829)) ([90bb6ba](90bb6ba)) Co-authored-by: ivov <ivov@users.noreply.github.com>

) and add support for cancelling ongoing operations inside a node. --------- Co-authored-by: Oleg Ivaniv <me@olegivaniv.com>

## [1.18.1](https://github.com/n8n-io/n8n/compare/n8n@1.18.0...n8n@1.18.1) (2023-11-30) ### Bug Fixes * **AWS DynamoDB Node:** Improve error message parsing ([#7793](#7793)) ([192770a](192770a)) * **core:** Prevent error messages due to statistics about data loading ([#7824](#7824)) ([c7d600a](c7d600a)) * **core:** Tighten checks for multi-main setup usage ([#7788](#7788)) ([060987a](060987a)) * **core:** Use AbortController to notify nodes to abort execution ([#6141](#6141)) ([dbad88d](dbad88d)) * **editor:** Disable context menu actions in read-only mode ([#7789](#7789)) ([ae25503](ae25503)) * **editor:** Fix cloud plan data loading on instance ([#7841](#7841)) ([b0039a3](b0039a3)) * **editor:** Fix credential icon for old node type version ([#7843](#7843)) ([9b0e2d1](9b0e2d1)) * **editor:** Fix icon for unknown node type ([#7842](#7842)) ([9cd0a75](9cd0a75)) * **editor:** Fix mouse position in workflow previews ([#7853](#7853)) ([83b5e6a](83b5e6a)) * **editor:** Suppress dev server websocket messages in workflow view ([#7808](#7808)) ([8a71178](8a71178)) * **Google Sheets Node:** Fix issue with paired items not being set correctly ([#7862](#7862)) ([95854db](95854db)) * **Google Sheets Node:** Read operation execute for each item ([#7800](#7800)) ([2563433](2563433)) * **HTTP Request Node:** Enable expressions for binary input data fields ([#7782](#7782)) ([7ec748a](7ec748a)) * **Microsoft SQL Node:** Prevent double escaping table name ([#7801](#7801)) ([6a8e7b1](6a8e7b1)) * **Notion Node:** Fix broken Notion node parameters ([#7864](#7864)) ([940944e](940944e)) Co-authored-by: netroy <netroy@users.noreply.github.com>

janober · 2023-11-30T11:45:25Z

Got released with n8n@1.18.1

This comment was marked as outdated.

Sign in to view

netroy force-pushed the cancel-wait-in-main-mode branch from 00f55f3 to 7570db5 Compare April 28, 2023 15:10

This comment was marked as outdated.

Sign in to view

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Apr 28, 2023

netroy force-pushed the cancel-wait-in-main-mode branch from 807d807 to 1fdbbe8 Compare May 2, 2023 23:57

netroy requested a review from krynble May 5, 2023 10:32

netroy force-pushed the cancel-wait-in-main-mode branch from 1fdbbe8 to faa1077 Compare May 5, 2023 10:32

netroy marked this pull request as ready for review May 5, 2023 10:33

This comment was marked as outdated.

Sign in to view

netroy closed this May 16, 2023

netroy reopened this Nov 10, 2023

netroy force-pushed the cancel-wait-in-main-mode branch 2 times, most recently from d2b66aa to 1e82cca Compare November 10, 2023 16:06

netroy mentioned this pull request Nov 14, 2023

feat(core): Update LLM applications building support (no-changelog) #7710

Merged

netroy force-pushed the cancel-wait-in-main-mode branch from 394f207 to 94f9d93 Compare November 14, 2023 18:43

netroy force-pushed the cancel-wait-in-main-mode branch 4 times, most recently from 4c09e9e to 1ee7c62 Compare November 23, 2023 14:17

netroy and others added 6 commits November 24, 2023 11:44

fix(core): Use AbortController to Notify nodes to abort execution

09d3f87

allow cancelling execution with wait nodes (with timeout less than 65…

d276b4c

… seconds)

Expose getExecutionCancelSignal getter to get AbortSignal

bd28ab2

Signed-off-by: Oleg Ivaniv <me@olegivaniv.com>

move executionCanceled flag to additionalData

df6ae1e

backport over pushMessage fixes

029c951

Let as many nodes listen to the abort signal, without getting the Max…

43c954d

…ListenersExceededWarning

netroy added 4 commits November 24, 2023 11:44

remove unnecessary changes

7ee3a30

handle cancellation properly

31a296f

restore getExecutionCancelSignal

4b9a419

handle cancel correctly

b7dfa0d

netroy force-pushed the cancel-wait-in-main-mode branch from 1ee7c62 to b7dfa0d Compare November 24, 2023 11:06

netroy commented Nov 24, 2023

View reviewed changes

ivov reviewed Nov 24, 2023

View reviewed changes

netroy added 4 commits November 24, 2023 16:17

Merge remote-tracking branch 'origin/master' into cancel-wait-in-main…

0a34be8

…-mode

make abortSignal optional for now

9f9c7a4

revert checks in markAsCrashed

3f1a3ee

revert changes in ActiveExecutions

7a736da

ivov previously approved these changes Nov 24, 2023

View reviewed changes

netroy removed the request for review from krynble November 24, 2023 16:06

✅: Fix 19-executions spec

12a506e

netroy dismissed ivov’s stale review via 4ef9fca November 24, 2023 16:21

change to warning toast

3e4a646

netroy force-pushed the cancel-wait-in-main-mode branch from 4ef9fca to 3e4a646 Compare November 24, 2023 16:43

OlegIvaniv approved these changes Nov 24, 2023

View reviewed changes

netroy merged commit d2c18c5 into master Nov 24, 2023
21 checks passed

netroy deleted the cancel-wait-in-main-mode branch November 24, 2023 17:17

github-actions bot mentioned this pull request Nov 29, 2023

🚀 Release 1.19.0 #7863

Merged

netroy added a commit that referenced this pull request Nov 29, 2023

fix(core): Use AbortController to notify nodes to abort execution (#6141

dbad88d

) and add support for cancelling ongoing operations inside a node. --------- Co-authored-by: Oleg Ivaniv <me@olegivaniv.com>

github-actions bot mentioned this pull request Nov 29, 2023

🚀 Release 1.18.1 #7872

Merged

janober added the Released label Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): Use AbortController to notify nodes to abort execution #6141

fix(core): Use AbortController to notify nodes to abort execution #6141

netroy commented Apr 28, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

OlegIvaniv commented Nov 15, 2023

netroy commented Nov 15, 2023

OlegIvaniv commented Nov 15, 2023

netroy Nov 24, 2023

ivov Nov 24, 2023

netroy Nov 24, 2023

ivov Nov 24, 2023

netroy Nov 24, 2023

ivov Nov 24, 2023

netroy Nov 24, 2023

ivov Nov 24, 2023

netroy Nov 24, 2023

ivov Nov 24, 2023

netroy Nov 24, 2023

ivov left a comment

github-actions bot commented Nov 24, 2023

cypress bot commented Nov 24, 2023 •

edited

Loading

github-actions bot commented Nov 24, 2023

janober commented Nov 30, 2023

		@@ -830,11 +820,16 @@ export class WorkflowExecute {
		let closeFunction: Promise<void> \| undefined;

		return new PCancelable(async (resolve, reject, onCancel) => {

fix(core): Use AbortController to notify nodes to abort execution #6141

fix(core): Use AbortController to notify nodes to abort execution #6141

Conversation

netroy commented Apr 28, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

OlegIvaniv commented Nov 15, 2023

netroy commented Nov 15, 2023

OlegIvaniv commented Nov 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 24, 2023

cypress bot commented Nov 24, 2023 • edited Loading

1 flaky test on run #3018 ↗︎

Review all test suite changes for PR #6141 ↗︎

github-actions bot commented Nov 24, 2023

janober commented Nov 30, 2023

cypress bot commented Nov 24, 2023 •

edited

Loading