Automattic / knox

S3 Lib
MIT License
1.74k stars 285 forks source link

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

Closed refaelos closed 11 years ago

refaelos commented 11 years ago

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 commented 11 years ago

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

Still can't find a solution to this issue.

nevill commented 11 years ago

@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 commented 11 years ago

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 https://github.com/LearnBoost/knox/issues/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 commented 11 years ago

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 commented 11 years ago

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.

nevill commented 11 years ago

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