search

asterisk / node-ari-client

Node.js client for ARI. This library is best effort with limited support.
Other
253 stars 99 forks source link

Throw error when host is not reachable #109

Closed jp1987 closed 6 years ago

jp1987 commented 6 years ago

Fixes #35

jp1987 commented 6 years ago

This is mostly based on #12, but that has been inactive for over 3 years now and we REALLY need this. I used pretty much the same logic, except for using a promise reject when catching the error, that seems to make the most sense to me. Would be great if we could get this merged.

chadxz commented 6 years ago

Just curious, is there any reason you're not bubbling up the request error as-is instead of wrapping it in another Error object?

chadxz commented 6 years ago

When swagger encounters an error it causes the library to emit an "APILoadError" event. Seems like it'd be good to keep that even if the host is not reachable.

jp1987 commented 6 years ago

Processed your feedback @chadxz

chadxz commented 6 years ago

is there any reason you're not bubbling up the request error as-is instead of wrapping it in another Error object?

^This still isn't clear to me. I haven't dug into the request implementation, but I wonder if assuming that the host is not reachable is the reason the error occurred is a correct assumption.

Also, I'm not entirely sure what sort of version bump this would be. Adding a new error scenario that wasn't there before seems to me to be a major version bump.

jp1987 commented 6 years ago

Sorry, I'm not sure what you mean by "I wonder if assuming that the host is not reachable is the reason the error occurred is a correct assumption". Can you elaborate?

I upped the version to 1.2.0 and updated the changelog.

chadxz commented 6 years ago

In your code you are treating any error from request as if it is caused by the host being unreachable. I'm asking- is this the right thing to do? Are there other classes of problems that the user may want to know about that we are re-casting as a "host not reachable" error?

jp1987 commented 6 years ago

Right, I see what you mean. I looked into the request lib and they appear to use the native http module, which has the following error codes (exhaustive list).

To me it makes the most sense to watch for ETIMEDOUT, ENOTFOUND and ECONNREFUSED. So I am filtering for these now.

jp1987 commented 6 years ago

ETIMEDOUT is a bit tricky to write a test case for (example here), which would require a bit of an overhaul of tests/helpers.js (it needs to return another client that simulates the timeout), so I've skipped that for now (basically all that it would test is the indexOf function, which the other 2 test cases now cover).

jp1987 commented 6 years ago

Can we get this merged then?

danjenkins commented 6 years ago

Who wants to merge and release?

chadxz commented 6 years ago

Sam's gonna do it

samuelg commented 6 years ago

Tagged and published to NPM.

chadxz commented 6 years ago

@jp1987 thank you for your contribution!