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

Prevent some potential unhandled exceptions #48

Merged
merged 4 commits into from Sep 11, 2021

Conversation

huntharo
Copy link
Contributor

@huntharo huntharo commented Sep 7, 2021

  • Source iterator next() functions can throw
  • There are 3 distinct cases where next() is called, one of which does not have a try/catch around it that will reject the outer promise, resulting in an unhandled promise rejection
  • Note: in the current state, the 3rd test fails - I'm not sure how you want to handle this... a try/catch for the entire next() function would handle it... but there are other ways, such as a try/catch within the catch...

- Source iterator next() functions can throw
- There are 3 distinct cases where next() is called, one of which does not have a try/catch around it that will reject the outer promise, resulting in an unhandled promise rejection
@huntharo huntharo changed the title Add test cases for source iterator exceptions Add (failing) test cases for source iterator exceptions Sep 7, 2021
@sindresorhus
Copy link
Owner

sindresorhus commented Sep 9, 2021

Thanks for catching this issue 🙏


such as a try/catch within the catch...

I think this would be best. Would you be able to add it?

@huntharo
Copy link
Contributor Author

Thanks for catching this issue 🙏

such as a try/catch within the catch...

I think this would be best. Would you be able to add it?

Done. I think the only option after an iterable.next() exception is to reject the promise and stop iteration... if we catch and disregard the exception and keep going we may just get infinite iterable.next() exceptions. As a result, I chose to reject the main promise and stop iteration in case of these exceptions to avoid an infinite loop and to try to preserve what I think is the existing behavior prior to catching these exceptions.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add (failing) test cases for source iterator exceptions Prevent some potential unhandled exceptions Sep 11, 2021
@sindresorhus sindresorhus merged commit 11bc75d into sindresorhus:main Sep 11, 2021
@sindresorhus
Copy link
Owner

Thanks :)

@huntharo huntharo deleted the test/corner-cases branch September 12, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants