timers cleanup

[gt3]

Current implementation of Timers (with setTimeout) keeps the host process running for the specified delay.

csp.timeout(ms)

alts - To reproduce, use this example from the docs. Run in Node repl.

And here's is my proposal to fix it without breaking changes:

1. Keep track of the timer along with the returned channel (in timeout)
2. Use the timer handle to call clearTimeout on channel close

There is one caveat with this approach. It introduces a new constraint on the system: Channels returned from timeout should be reserved for timeout, except now you can explicitly close it, which will eventually release timers.

Node timers have ref/unref which I'll investigate further. However clearTimeout seems to be more fitting and universal.

thoughts? .. @ubolonton @hung-phan

[hung-phan]

Seem interesting. Is there any use case we want to do that? We can close the channel before setTimeout being triggered, but is there any reason we want this optimization? For me, call ch.close() multiple time won't affect the logic at all. 🤔

[gt3]

Just to clarify, channel closing should be done explicitly by the user, when required, which should indirectly (automatically) clear the timer.

I was trying to use alts to manage async sources with a request timeout when I stumbled upon this error. Jest tests took forever to complete since there was alts + timeout in my test suite. Then I tried alts in Node with similar findings. Btw - could someone pls confirm my findings? Increase the timeout to make results more evident.

Since we are returning channel from timeout API, and managing/calling timeout callback internally, it would be a good idea to clean it up. I see it as dangling code.

Of course, the other option would be to return timer handle with the channel for more control but that would take away conciseness of timeout. Plus I feel that staying close to Clojure syntax-wise is a good thing.

[hung-phan]

Don't quite understand the purpose behind this improvement. Is there any reason we want this instead of just passing test?

https://github.com/clojure/core.async/blob/master/src/main/clojure/cljs/core/async/impl/dispatch.cljs#L35

[gt3]

@hung-phan I've created a gist to demonstrate my point but I think you get the idea. If we are closing the channel why not clear the timeout?

I am not sure why it isn't handled in cljs. In the link you've described timer handle is returned from queue-delay but that's the only difference I could find.
https://github.com/clojure/core.async/blob/f8e87e1625b1660b7f3b0aea044aad1327441741/src/main/clojure/cljs/core/async/impl/timers.cljs#L152

[gt3]

Here's the gist: https://gist.github.com/gt3/a7db07ef7a0db0dd6c1bcd0fef2ec6a8

from the example described in the docs: https://github.com/ubolonton/js-csp/blob/master/doc/basic.md#yield-altsoperations-options

[hung-phan]

The example in the gist you gave is different than your point. This is the first channel in yield csp.timeout(1000);, and this is the second channel const cancel = csp.timeout(300);. Why do we need to close (or stop) the first channel when the second channel is close?

If you need an example, it can be like this.

require('babel-register');

var csp = require('../../src/csp');

const inputCh = csp.chan();
const cancelCh = csp.timeout(1000);

csp.go(function *() {
  yield csp.timeout(200);
  cancelCh.close();
});

csp.go(function *() {
  yield csp.timeout(400);
  yield csp.put(inputCh, 10);
});

csp.go(function *() {
  const ch = yield csp.alts([inputCh, cancelCh]);
  console.log(ch.channel === cancelCh); // => true
});

The last go block will be true.

[hung-phan]

I can understand the above situation why we want to clear the timer when we try to close the channel? However, the only use case for this improvement is process.on('exit'), and the logic is not affected.

require('babel-register');

const csp = require("../../src/csp");
const cancelCh = csp.timeout(1000);
let startTime = +(new Date);
let endTime;
let shouldEndTime;

csp.go(function *() {
  yield cancelCh;
  shouldEndTime = +(new Date);
  // do sth else
});

csp.go(function *() {
  yield csp.timeout(200);
  cancelCh.close();
  console.log('cancelCh is closed: ', +(new Date) - startTime);
});

process.on('exit', () => {
  endTime = +(new Date);
  console.info("end: %dms \t should-end: %dms", endTime - startTime, shouldEndTime - startTime);
});

// cancelCh is closed:  519
// end: 1479ms      should-end: 521ms

[gt3]

@hung-phan It is more about fixing the bug than addressing a use case. Our code should be able to run in the target environment without side effects. As it stands right now, select/alts + timeout produces unexpected behavior. I tried to use the example from the docs but yours is more fitting. Thanks for sharing it.

[gt3]

@hung-phan we should resolve this bug, it is coming in the way more often for us in production

see pr: #111