search

mreinstein / node-gearman

⚙ Gearman client and worker for node
75 stars 13 forks source link

Please provide a timeout and error callback #8

Closed goelvivek closed 11 years ago

goelvivek commented 11 years ago

Currently in module code , It is doing 

   this._conn.on('error', function(error) {
        return console.log('error', error);
      });
      this._conn.on('close', function(had_transmission_error) {
        return console.log('socket closed');
      });
      this._conn.on('timeout', function() {
        return console.log('socket timed out');
      });

There will no way for external application to know about timeout. There should be two callback for onError and onTimeout.

mreinstein commented 11 years ago

good idea!

goelvivek commented 11 years ago

should I submit the patch ? On 19-Jun-2013 10:42 PM, "Mike Reinstein" notifications@github.com wrote:

good idea!

— Reply to this email directly or view it on GitHubhttps://github.com/mreinstein/node-gearman/issues/8#issuecomment-19698561 .

mreinstein commented 11 years ago

If you have time I'd love to get a patch, sure! But if not I'll bang one out later this evening.

How are you thinking you'd go about it? My thoughts are that I'd add another argument on the end of the constructor for an optional callback function

goelvivek commented 11 years ago

Yes. I am going to use same approach. On 19-Jun-2013 10:47 PM, "Mike Reinstein" notifications@github.com wrote:

If you have time I'd love to get a patch, sure! But if not I'll bang one out later this evening.

How are you thinking you'd go about it? My thoughts are that I'd add another argument on the end of the constructor for an optional callback function

— Reply to this email directly or view it on GitHubhttps://github.com/mreinstein/node-gearman/issues/8#issuecomment-19698827 .

mreinstein commented 11 years ago

sounds great, thanks for the help!

goelvivek commented 11 years ago

As eventemitter is already used in module. Will it not better to use that for these events? On 19-Jun-2013 10:50 PM, "Mike Reinstein" notifications@github.com wrote:

sounds great, thanks for the help!

— Reply to this email directly or view it on GitHubhttps://github.com/mreinstein/node-gearman/issues/8#issuecomment-19699075 .

mreinstein commented 11 years ago

good point. I forgot about that. It's been a while since I touched the code so I didn't remember if I extended emitter or not.

goelvivek commented 11 years ago

I will submit the using event by tomorrow. On 19-Jun-2013 11:00 PM, "Mike Reinstein" notifications@github.com wrote:

good point. I forgot about that. It's been a while since I touched the code so I didn't remember if I extended emitter or not.

— Reply to this email directly or view it on GitHubhttps://github.com/mreinstein/node-gearman/issues/8#issuecomment-19699777 .

goelvivek commented 11 years ago

Sorry for delay. I have submitted one pull request. Can you please have a look ?