hemerajs / hemera

🔬 Writing reliable & fault-tolerant microservices in Node.js https://hemerajs.github.io/hemera/
MIT License
806 stars 70 forks source link

Act callback called twice on TimeoutError #32

Closed acehko closed 7 years ago

acehko commented 7 years ago

If a timeout error occurs when a service is taking a longer time to respond the act callback is called twice. Once when the timeout error occurs and the second time when the service has finished the request: Example:

hemera.add({ topic : 'test', cmd : 'timeout' }, (msg, reply) => {
    setTimeout(() => { reply(null, { ok : true }) }, 3000);
});

hemera.ready(() => {

    hemera.act({ topic : 'test', cmd : 'timeout', timeout$ : 1000 }, (err, res) => {
        console.log('done');
        console.log(err || res);
    });

});

Output:

done
{ TimeoutError
    at /home/andrija/Documents/hemera-test/node_modules/nats-hemera/build/index.js:949:21
    at Timeout._onTimeout (/home/andrija/Documents/hemera-test/node_modules/nats/lib/nats.js:1165:41)
    at ontimeout (timers.js:365:14)
    at tryOnTimeout (timers.js:237:5)
    at Timer.listOnTimeout (timers.js:207:5)
  message: 'Timeout',
  pattern: { topic: 'test', cmd: 'timeout', 'timeout$': 1000 } }
done
{ ok: true }
StarpTech commented 7 years ago

Hi @acehko thanks for reporting. What is your expecting behaviour? If you fire a request and your client timeout is too low then you can run into a timeout issue but this does not implicit that the add won't ping back. In this case you have to increase the $timeout for you action. This behaviour is normal because we use timeouts to ensure that a service works correctly. You have to remember that we work with a pubsub system (NATS). On the section https://github.com/hemerajs/hemera#handle-timeout-errors I documented this.

acehko commented 7 years ago

@StarpTech My exepected behaviour was at first that it should only call the callback once. I first noticed it in hemera but after a test it seems it is the same with seneca (redis-queue transport). After thinking a bit I think the current behavior is good. It just felt strange when I noticed it. PS: Is it possible to define a global timeout for all acts?

Edit: I also noticed that the timeout error is not triggering any of the available response events

StarpTech commented 7 years ago

@acehko I also thinking about the current behaviour and it is possible but we have to implement a add timeout which has the same value as the requestor timout. The requestor timeout can be transfered over the meta property but this raise the question is this the correct behaviour even when the request respond correctly? As you said this is the normal behaviour. I think we should't break this and treat it as a problem for the implementer. Increase the timeout if you dont know how long your action can take.

Is it possible to define a global timeout for all acts?

Yes

new Hemera(nats, { timeout: 5000 })

I also noticed that the timeout error is not triggering any of the available response events

Timeouts happens on client side. You get no response back the request will be handled as failed.

acehko commented 7 years ago

@StarpTech I agree that you shouldn't break the current behaviour. But I think that a TimeoutError should be treated as a response and should trigger an event. For example a BusinessError would trigger the onClientPostRequest error. The reason is that I use the events to log all requests/responses but with the current behaviour the timeout will not be looged automatically.

StarpTech commented 7 years ago

That make sense. I will investigate in it.

acehko commented 7 years ago

Thanks. I will close this as the original issue was resolved.

StarpTech commented 7 years ago

@acehko is implemented see https://github.com/hemerajs/hemera/commit/2b98e050d1e2742fefa41ff6859a54e26dbed981#diff-ee2c2206b5ed08491be6674035f0612a

StarpTech commented 7 years ago

Hi @acehko I changed the behaviour because NATS has support for this. It is documented here https://hemerajs.github.io/hemera/1_request_reply.html

As of now any request will process exact one response. If you want to get multiple responses you can use the maxMessages$ attribute.

acehko commented 7 years ago

Hi @StarpTech, thank you for the update. maxMessages$ looks interesting, it could be useful. But does this mean that the callback will now be called only once? If the TimeoutError is generated on the client, I assume the callback will still be called after the service replies

StarpTech commented 7 years ago

@acehko it means that you can receive only one message but the callback can be called multiple times.