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

If knox gets timeout, node.js server process gets killed #201

Closed
refaelos opened this issue Oct 2, 2013 · 6 comments
Closed

If knox gets timeout, node.js server process gets killed #201

refaelos opened this issue Oct 2, 2013 · 6 comments

Comments

@refaelos
Copy link

refaelos commented Oct 2, 2013

Hope i'm not duplicating anything but when i try to get many files from S3 the server gets killed randomly. It works most of the time but sometimes when a socket fails the whole server is dead.

The error is:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: read ECONNRESET
    at errnoException (net.js:901:11)
    at TCP.onread (net.js:556:19)
@refaelos
Copy link
Author

refaelos commented Oct 2, 2013

Just saw #116 and it looks like the issues are related.

Still can't find a solution to this issue.

@nevill
Copy link

nevill commented Oct 7, 2013

@refaelos No, this issue is caused by an uncaught error event, as noted here:

Error events are treated as a special case in node. If there is no listener for it, then the default action is to print a stack trace and exit the program.

I guess you called putFile without handling the error event as me. The simple fix is to handle it.

var emitter = knoxClient.putFile(...)

emitter.once('error', errorHandler)

@nevill
Copy link

nevill commented Oct 7, 2013

I don't understand why putFile here is so different from other methods, like putStream, get, head etc. All the others return a request object, only this one return an EventEmitter object.

According to git log, it's introduced by the fix of #191.

I would suggest to make putFile as same as putStream, get rid of the emitter.
The client code can handle the progress, error directly if they want.

Beside, we already have registerReqListeners also listen on error, don't need to listen to it again on line 417, the function wrappedFn seemed useless.

@domenic
Copy link
Contributor

domenic commented Oct 7, 2013

We cannot return the req because we need to do the fs.stat before the req is created. Thus we forward things from req to the returned event emitter once req is available.

@domenic
Copy link
Contributor

domenic commented Oct 14, 2013

It seems like this bug was just not attaching an error handler to the returned emitter. I am starting to think we should swallow any errors that occur after the first one though.

@domenic domenic closed this as completed Oct 14, 2013
@nevill
Copy link

nevill commented Oct 14, 2013

Swallowing any errors sounds like a good idea, but I am afraid it might be a bug because I can see the same error twice. Not sure if it's the same thing as you mentioned in #1

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

No branches or pull requests

3 participants