Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: EmailReadImap unhandled promise rejection #2485

Closed
wants to merge 1 commit into from

Conversation

tennox
Copy link
Contributor

@tennox tennox commented Nov 26, 2021

EmailReadImap swallows errors & in log I see this (this issue is not about the errors, but error handling):

n8n_1  | (node:60) UnhandledPromiseRejectionWarning: Error: Unexpected search option: HEADER.CONTENT-TYPE "MULTIPART/MIXED"
n8n_1  |     ...
n8n_1  |     at ImapSimple.search (/usr/local/lib/node_modules/n8n/node_modules/imap-simple/lib/imapSimple.js:127:12)
n8n_1  |     at getNewEmails (/usr/local/lib/node_modules/n8n/node_modules/n8n-nodes-base/dist/nodes/EmailReadImap/EmailReadImap.node.js:219:46)
n8n_1  |     at Connection.onmail (/usr/local/lib/node_modules/n8n/node_modules/n8n-nodes-base/dist/nodes/EmailReadImap/EmailReadImap.node.js:344:50)
n8n_1  |     ...
n8n_1  | (node:60) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 2)
n8n_1  | (node:60) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
n8n_1  | (node:99) UnhandledPromiseRejectionWarning: NodeOperationError: Custom email config is not valid JSON.
n8n_1  |     at Connection.onmail (/usr/local/lib/node_modules/n8n/node_modules/n8n-nodes-base/dist/nodes/EmailReadImap/EmailReadImap.node.js:337:39)
n8n_1  |     ...
n8n_1  | (node:99) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
n8n_1  | (node:99) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I have attempted to fix this by:

  • config parsing: move outside the onmail handler (which means the thrown error should be catched normally)
  • IMAP errors: emitting the error instead of throwing it

but:

  • I did not manage to get my development environment to work (ERROR: LoggerProxy not initialized)
  • I did not find documentation on how to correctly emit errors, so I did this:
    this.emit([[{
      json:{}, // seems like I have to pass it
      error: new NodeOperationError(this.getNode(), `IMAP query failed: ${error instanceof Error ? error.message : error}`)
    }]]);

Related to #2091 (but only partially)

@CLAassistant
Copy link

CLAassistant commented Nov 26, 2021

CLA assistant check
All committers have signed the CLA.

@ivov ivov added node/improvement New feature or request community Authored by a community member labels Dec 15, 2021
@krynble
Copy link
Contributor

krynble commented Mar 15, 2022

Hey @tennox

Thank you for your contribution. Sorry about the delay in getting back to you.

The first change you made, moving the parsing of the options upper makes total sense to me and helps you more easily find possible errors.

What worries me a bit is about the second change where the error property exists, but it will not cause the execution to display as an Error but as successful instead.

This property is meant for a future design and currently won't have the fully expected behavior, which is causing n8n to show this execution as an error.

I made some small changes and will be closing this PR in favor of https://github.com/n8n-io/n8n/pull/2991/files

@krynble krynble closed this Mar 15, 2022
@janober
Copy link
Member

janober commented Mar 19, 2022

Thanks a lot for your contribution. Got merged with #2991

@janober janober added the Upcoming Release Will be part of the upcoming release label Mar 19, 2022
@janober
Copy link
Member

janober commented Mar 20, 2022

Got released with n8n@0.169.0

@janober janober removed the Upcoming Release Will be part of the upcoming release label Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member node/improvement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants