Error on initialization should prevent actual HTTP request from being sent

[arcana261]

Hello

Working on a propitiatory piece of app the other day, I ran into a strange exception happening on nextTick. This exception would complain of can not call method request of undefined. When I inspected the problem deeply I understood the problem was that the URI which I passed to request did not have any protocol defined, like /bad-uri but also the problem seemed to be triggered because a stream was piped to request to upload some files to a WebDAV server. I also confirmed that request did in fact call my callback with provided exception error but was still trying to make the actual HTTP request regardless. The exception happened because initialization was failed thus leaving undefined variables behind but this case of error was never checked in Request.write method which was called in nextTick following stream of data from my file. So I tried to fix this problem by:

1. Creating a test case (which needed tape-catch to fail tests creating exceptions on process level)
2. Setting self._aborted to true on every error inside request
3. Checking self._aborted in methods Request.write and Request.end
4. Fixing a test case in test-bearer-auth that falsely relied on request making HTTP requests regardless of errors in it's initialization

The following is an image showing the stacktrace of exception which I'm talking about:

[simov]

Hey, @arcana261, very good work on this PR 👍

However I'm proposing a slightly simpler solution here: simov@6931cea

Basically you need only the self.abort() call on line 284, then I just changed the check in write and end to be slightly more readable if (self._aborted) {return}.

Unfortunately even this doesn't cover all possible cases, and our error handling is kind of awkward due to the way the module is being initialized. You can read about it here: #1558

Still I think that aboring on line 284 is slightly better for this particular case, because it says fatal error anyway. Your solution is not bad either, but it's going to work only if you have a callback, which unfortunately is not always the case.

Let me know what do you think.

[arcana261]

Dear good Sir @simov. Sorry for replying so late... (holidays in my country!)

I think it is smart... I'm just sorry for missing the abort method 😄 Regarding the callback, I would suggest we take a default callback if none was provided that throws an exception or just simply prints out in the console. Still I have two questions regarding your simov/request@6931cea

1. Why would you replace-back tape instead of tape-catch. Because the test won't fail using just tape
2. t.equal(numBearerRequests, 13) fails in auth-bearer test because the HTTP request was not sent due to aborting it

I would definitely say this will not cover all our errors but I think we are making an improvement here 👍 Of course we are not expected to put a big try-catch on whole initialization anyway haha! I think that taking a default callback sounds more fun to me! (I do this all the time!) and I just hope I will be of help in finding more bugs in the future 😄

[simov]

Providing a callback is optional, and having one means that the response will be buffered into memory - not something that you want to have by default.

Also I don't want to have the _abort flag set in the error event handler. Because that flag is used in slightly different context, that's why I prefer calling the abort method explicitly for that type of error. And having the error handler for the callback case won't work when you are streaming.

No idea why you used the tape-catch module in the first place. Search for t.throws in the test suite to see how you can test code that throws.

As for the auth-bearer test - like I said, our error handling is far from perfect at the moment. With my change the request is being aborted only when the URI is invalid, that's why this test passes.

Happy holidays! 🎉

[arcana261]

Ahh I see... Thank you for your patient response. So we need to abort() the error in which that test generates maybe? and BTW I used the tape-catch because the error is actually generated in nextTick which I think throws would not be able to detect.

Thanks! 😊

[simov]

re: nextTick that makes sense.

So we need to abort() the error in which that test generates maybe?

I don't understand your question.

What I'm suggesting is to merge my last commit on top of yours. Quoting the relevant bits from my previous comment:

Also I don't want to have the _abort flag set in the error event handler. Because that flag is used in slightly different context, that's why I prefer calling the abort method explicitly for that type of error. And having the error handler for the callback case won't work when you are streaming.

[arcana261]

Ok... Thanks! I appreciate it 😄

[simov]

👍