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(adapter-node-http): Fix unhandled rejection if connection fails #160

Merged
merged 1 commit into from
Jan 16, 2019

Conversation

dustinsoftware
Copy link
Contributor

An unhandled promise rejection was thrown if a socket could not be opened,
which prevents handling the error gracefully in a consuming app.

First of all, thanks for building this library! I'm exposing a HTTP API on top of it here and it's working pretty well. However..

I'm running into a crash when polly can't connect to an API (port isn't open, for instance).

In my app, I'm tearing down the process if there are any unhandled promise rejections by using:

process.on('unhandledRejection', exception => {
	throw exception;
});

I had to fix promise rejection handling in a few places, but it works properly now. I've confirmed that this fix allows polly-proxy to return a graceful HTTP 500 from the express instance instead of crashing the app, but am unsure the best way to add a test for this type of bug.

@dustinsoftware
Copy link
Contributor Author

Looks like CI passed for Node 8 and 10, but failed for 6 during package restore 🤷‍♂️

@dustinsoftware
Copy link
Contributor Author

I've added a test that verifies the expected behavior in polly-proxy on the test-updates branch.

To repro with that project:
yarn build && yarn test:ci

To verify this fix:
yarn link @pollyjs/adapter-node-http
yarn link @pollyjs/core
yarn build && yarn test:ci

I'm not sure that adding promise.catch(() => {}) is the best solution here, although it doesn't seem to negatively affect consumers (such as flush)

@dustinsoftware
Copy link
Contributor Author

FWIW I was able to successfully hack around the issue in deferred-promise.js but ended up forking adapter-node-http until we can get a fix in :)

dustinsoftware/polly-proxy@c558c6c

Copy link
Collaborator

@offirgolan offirgolan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting these fixes @dustinsoftware! The changes look good but im skeptical on one part.

@dustinsoftware dustinsoftware force-pushed the fix-node-http-crash branch 2 times, most recently from 668b5b3 to c1f9dac Compare January 14, 2019 17:21
An unhandled promise rejection was thrown if a socket could not be opened,
which prevents handling the error gracefully in a consuming app.
@offirgolan offirgolan added bug 🐞 Something isn't working 📦 adapter-node-http labels Jan 15, 2019
@offirgolan offirgolan requested a review from jasonmit January 16, 2019 00:28
@jasonmit jasonmit merged commit 12fcfa7 into Netflix:master Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working 📦 adapter-node-http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants