repeaterjs / repeater

The missing constructor for creating safe async iterators
https://repeater.js.org
MIT License
459 stars 12 forks source link

Way of checking if channel is closed #21

Closed elderapo closed 5 years ago

elderapo commented 5 years ago

Currently, there is no synchronous and easy way of checking if the channel is closed. It would be very useful for debugging purposes. The best current solution I can think of is checking if stop promise has been resolved but this seems both hacky and there is no synchronous way of checking it.

There is however a question if allowing this is a great idea. I think it might make it easier to write some buggy/leaky code. For example, I was merging some channels in a very specific way and needed to act accordingly when other channels got closed, etc.

The API I suggest is channel.isClosed/channel.isClosed().

brainkim commented 5 years ago

It’s an explicit design goal of channels to not deviate from async generators in behavior or API. Async generators are not synchronously queryable using a property or method without consuming the next result via the next method and checking the done property. The question I have is, what advantage do you have in having a closed property over 1. storing the previous iterator result and checking its done property, or 2. calling next again and checking the done property. Alternatively, you could also use for await with a closed iterator and there will be no effect (it’s the same as doing for (const value of []).

I am 99% sure that this is a wontfix issue unless we can come up with an incredibly compelling use case that could not be handled using Channel.prototype.next or for await. Abiding by the async iterator API and making channels indistinguishable from async generators is immensely valuable insofar as we can then take functions which return channels and treat them like they were async generator functions.

elderapo commented 5 years ago
  1. I would say channel.isClosed() is more convenient than storing the last result of next and checking if it returned done.

  2. I am not sure but I think that calling channel.next() at the same time as running for await loop on the same specific channel might cause that some data does not make it to for await loop? Please correct me if I am wrong.

What I needed it for was something like this. It had more logic but overall was similar.

const someChannel1 = new Channel<any>(() => {
  // ...
})

const someChannel2 = new Channel<any>(() => {
  // ...
})

const newChannel = new Channel<number>(async (push, stop) => {

  const isNewChannelClosed = false; // newChannel.isClosed();

  (async () => {
    for await (const something1 of someChannel1) {
      if (isNewChannelClosed) {
        // do x
      }

      push(something1)
    }
  })();

  (async () => {
    for await (const something2 of someChannel2) {
      if (isNewChannelClosed) {
        // do y
      }

      push(something2)
    }
  })()

  await stop()
});

Not the best/cleanest code I've written >.<

brainkim commented 5 years ago

Ah so the tricky thing with your code is that:

  1. newChannel isn’t accessible within the channel executor
  2. even if newChannel was accessible,isNewChannelClosed would become stale when the two for await loops run.

If you want to query the closed state of the channel within the executor you have to use the stop argument as a promise, along with a variable:

const newChannel = new Channel<number>(async (push, stop) => {

  let isNewChannelClosed = false;
  stop.then(() => (isNewChannelClosed = true));

  (async () => {
    for await (const something1 of someChannel1) {
      if (isNewChannelClosed) {
        // do x
      }

      push(something1)
    }
  })();

  (async () => {
    for await (const something2 of someChannel2) {
      if (isNewChannelClosed) {
        // do y
      }

      push(something2)
    }
  })();
});

I use this technique a lot in the code for channel combinators. See for example the implementation of Channel.merge: https://github.com/channeljs/channel/blob/master/packages/channel/src/channel.ts#L486-L524