qbit / node-pushover

Pushover notifications for node.js (JavaScript,NodeJS)
ISC License
163 stars 24 forks source link

Fixed error handling when https-request fails early #12

Closed sweetpi closed 10 years ago

sweetpi commented 10 years ago

Thanks for this cool node module. I had some problems when I tried to send push notifications while my network was down:

node ./test/test-onerror.js 
token=xxx&user=xxx&message=omg%20node%20test&sound=magic&title=Well%20-%20this%20is%20fantastic&device=&url=&url_title=&priority=&timestamp=

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: getaddrinfo ENOTFOUND
    at errnoException (dns.js:37:11)
    at Object.onanswer [as oncomplete] (dns.js:124:16)

So the error didn't get catched and brought node down :(. I fixed the error handling by moving the error listener from the response object to the request object.

from: http://nodejs.org/api/http.html#http_http_request_options_callback "If any error is encountered during the request (be that with DNS resolution, TCP level errors, or actual HTTP parse errors) an 'error' event is emitted on the returned request object."

qbit commented 10 years ago

Thanks for the diff! Did you by chance make it against an old fork?

sweetpi commented 10 years ago

Oh sorry this was by accident. Pulled the current version. It seems that there are is a mix of spaces and tabs, what do you prefer?

qbit commented 10 years ago

Sweet! Spaces - but I will go in and fix that after I merge your pull req!

sweetpi commented 10 years ago

Now it would be able to merge, but the diff still looks strange because of some space/tab changes. Is it ok four you, or should I make a new patch?

qbit commented 10 years ago

Thanks! Just pushed version 0.2.1 to npm!

sweetpi commented 10 years ago

Thanks!