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

Error handling bug #3

Closed
karlroberts opened this issue Aug 31, 2016 · 4 comments · Fixed by #4
Closed

Error handling bug #3

karlroberts opened this issue Aug 31, 2016 · 4 comments · Fixed by #4

Comments

@karlroberts
Copy link
Contributor

Hi @aluxian,

Thanks for the gulp task, I've used it and it now works for me, but I thought I'd bring an issue to your attention that took up most of my day to debug.

The root cause of this was my github token was revoked but as you can see from the error below I had no way of seeing that, in fact it looked OK "Release created successfully.

The problem I think was two-fold.

The useless error message was because in the dependant publish-release npm lib, that you are a contributor to, is an EventEmitter and yet does not emit the "error" event... it instead calls callback(err) at the end.

it could have called self.emit('error', err) . When I monkey patched that in before callback(err) this gulp task at least showed me the 401 not authorised message at the root cause.

I'm not sure what you would do with the callback of an EventEmitter that has an error? maybe just call callback()? My Node-fu is not that strong yet.

  1. Where did the error happen? How did it slip through?
    publish-release relies on request to do the http. request does not treat response code 401 as an error (unlike angular.js http). Consequently we see that
    self.emit('created-release') on line 81 is called even though it is after a test for err. I think err is just for timeouts or network issues in request, so you need to check that res.status is not >=400 && < 600.

because of this a spurious body (containing the error message 401) is passed to down the chain to uploadAssets. I reckon that somewhere in there an error occurs that is not trapped because the body is not as expected and this bubbles up to the outer callback. (just a guess). NB note that the opts.reuseRelease code block doesn't suffer from 401 not being seen as an error when it checks statusOk, but unfortunately if status is not OK it calls requestCreateRelease which does not handle the bad status correctly.

Anyway I hope that lets you harden it up with better error messages, after fixing my token the gulp-github-release worked and uploaded artefacts so thanks for the code :-)

Cheers
Karl

see error below:-

[18:18:15] Release created successfully at https://github.com/suited/suited.js/releases/tag/1.0.11-alpha+bd.2016-8-31-181759-000.commitish.c86dd46a4d383ae3083df01a38d2b99fee2f0382 events.js:87 throw Error('Uncaught, unspecified "error" event.'); ^ Error: Uncaught, unspecified "error" event. at Error (native) at DestroyableTransform.emit (events.js:87:13) at done (/home/robertk/projects-oss/suited.js/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:168:25) at /home/robertk/projects-oss/suited.js/node_modules/through2/node_modules/readable-stream/lib/_stream_transform.js:116:7 at /home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/index.js:161:21 at taskCallback (/home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/node_modules/async/lib/async.js:468:21) at Array.uploadAssets (/home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/index.js:116:77) at listener (/home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/node_modules/async/lib/async.js:490:46) at /home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/node_modules/async/lib/async.js:441:17 at _each (/home/robertk/projects-oss/suited.js/node_modules/gulp-github-release/node_modules/publish-release/node_modules/async/lib/async.js:46:13)

@aluxian
Copy link
Owner

aluxian commented Aug 31, 2016

Thanks! This is good to know for anyone using this project.

Unfortunately, I don't have time to improve it now. If anyone wants to send a PR, they're welcome.

Thanks for writing about it! I'll link it in the README.

@karlroberts
Copy link
Contributor Author

I have fixed it. needs a 'error' handler in this gulp plugin and a fix upstream in publish-release to emit errors and handle 400 -> 599 status codes.

I will submit the Pull Requests in both projects

Cheers
Happy Coding
Karl

karlroberts added a commit to karlroberts/gulp-github-release that referenced this issue Sep 1, 2016
The upstream dependency 'publish-release' also requires a fix to emit 'error' as it is an 'EventEmitter'
@aluxian aluxian closed this as completed in #4 Sep 1, 2016
aluxian pushed a commit that referenced this issue Sep 1, 2016
Fix for :- Error handling bug issue #3.
@aluxian
Copy link
Owner

aluxian commented Sep 1, 2016

I published both packages to npm

@karlroberts
Copy link
Contributor Author

Yep. Thanks. Using it already 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants