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

Act callback called twice on TimeoutError #32

Closed
acehko opened this issue Feb 6, 2017 · 10 comments
Closed

Act callback called twice on TimeoutError #32

acehko opened this issue Feb 6, 2017 · 10 comments

Comments

@acehko
Copy link

acehko commented Feb 6, 2017

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
Copy link
Member

StarpTech commented Feb 6, 2017

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
Copy link
Author

acehko commented Feb 6, 2017

@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
Copy link
Member

StarpTech commented Feb 6, 2017

@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
Copy link
Author

acehko commented Feb 7, 2017

@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
Copy link
Member

That make sense. I will investigate in it.

@acehko
Copy link
Author

acehko commented Feb 7, 2017

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

@acehko acehko closed this as completed Feb 7, 2017
@StarpTech
Copy link
Member

@acehko is implemented see 2b98e05#diff-ee2c2206b5ed08491be6674035f0612a

@StarpTech
Copy link
Member

StarpTech commented Mar 8, 2017

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
Copy link
Author

acehko commented Mar 14, 2017

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
Copy link
Member

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

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

2 participants