Skip to content
This repository

Fix uncatchable errors. Update tests/test-errors.js for coverage. #365

Closed
wants to merge 1 commit into from

2 participants

Dan MacTough Mikeal Rogers
Dan MacTough

Request#init has two places where it will emit an 'error' event, but listeners have not had a chance to attach yet. So any emitted errors actually throw.

Tests included.

Example:

request('/invalid').on('error', function (e){
  console.error('Hey... I should have been caught!');
})
// Throws
Error: Invalid URI "/invalid"
Dan MacTough danmactough Fix uncatchable errors. Update tests/test-errors.js for coverage.
Request#init has two places where it will emit an 'error' event, but listeners have not had a chance to attach yet. So any emitted errors actually throw.

Tests included.
47626f4
Mikeal Rogers
Owner
Dan MacTough
Mikeal Rogers
Owner

you're misunderstanding the purpose of the validation. it's impossible to validate all inputs before using them, many can still fail when passed to a node core API during construction and will throw. the reason we do a validation check instead of passing directly to url.parse (which would throw, which is what request used to do) is so that we can emit a catchable error for redirects.

it's on the user to validate inputs passed to request construction. if we go down this rabbit hole then all of construction would be wrapped in a try/catch block. it is request's responsibility to validate the inputs of forwards and avoid throwing.

the idea here is that users know more about what they are passing to an API than the creator of the API and are in a better position to write fast code that can check it for problems. if a user passes invalid input it's request's responsibility to error as soon as possible.

passing construction errors to a deferred error handler would encourage them to be unhandled. the error listener is for errors in the future which are the result of some amount of IO. you can assume error's emitted are because of something a remote server did (either disconnected abruptly or returns invalid headers) and is less of an issue with your code.

this is an important distinction. one of these error types (throws) is your fault and errors that get emitted are likely do to something outside of your code on the network.

Dan MacTough
Dan MacTough danmactough referenced this pull request in danmactough/node-feedparser
Closed

"Unexpected End" (uncaught error) #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Nov 07, 2012
Dan MacTough danmactough Fix uncatchable errors. Update tests/test-errors.js for coverage.
Request#init has two places where it will emit an 'error' event, but listeners have not had a chance to attach yet. So any emitted errors actually throw.

Tests included.
47626f4
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 25 additions and 3 deletions. Show diff stats Hide diff stats

  1. +9 3 main.js
  2. +16 0 tests/test-errors.js
12 main.js
@@ -133,8 +133,12 @@ Request.prototype.init = function (options) {
133 133 }
134 134
135 135 if (!self.uri) {
136   - // this will throw if unhandled but is handleable when in a redirect
137   - return self.emit('error', new Error("options.uri is a required argument"))
  136 + // this is handleable when in a redirect, but not on initial request
  137 + // unless fired on process.nextTick
  138 + process.nextTick(function () {
  139 + self.emit('error', new Error("options.uri is a required argument"))
  140 + })
  141 + return // This error was fatal
138 142 } else {
139 143 if (typeof self.uri == "string") self.uri = url.parse(self.uri)
140 144 }
@@ -167,7 +171,9 @@ Request.prototype.init = function (options) {
167 171 // he should be warned that it can be caused by a redirection (can save some hair)
168 172 message += '. This can be caused by a crappy redirection.'
169 173 }
170   - self.emit('error', new Error(message))
  174 + process.nextTick(function () {
  175 + self.emit('error', new Error(message))
  176 + })
171 177 return // This error was fatal
172 178 }
173 179
16 tests/test-errors.js
@@ -34,4 +34,20 @@ try {
34 34 assert.equal(e.message, 'Body attribute missing in multipart.')
35 35 }
36 36
  37 +try {
  38 + request('/invalid').on('error', function (e) {
  39 + assert.equal(e.message, 'Invalid URI "/invalid"')
  40 + });
  41 +} catch (e) {
  42 + assert.fail("Should not throw");
  43 +}
  44 +
  45 +try {
  46 + request('').on('error', function (e) {
  47 + assert.equal(e.message, 'options.uri is a required argument')
  48 + });
  49 +} catch (e) {
  50 + assert.fail("Should not throw");
  51 +}
  52 +
37 53 console.log("All tests passed.")

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.