Making push synchronous

[elderapo]

I am not sure why push is asynchronous. From a quick look at the code, it seemed like there are only wrappers like Promise.resolve(value).

It complicated things because you have to either use await with every push which leads to more complex code (you have to consider that other things might execute meanwhile there is a push to the channel) or omit the await and just treat it as a synchronous call which makes eslint go nuts because it doesn't want to allow floating promises.

[brainkim]

Push is meant to be used synchronously in most cases yes, and it will throw synchronous overflow errors if there are too many pending messages because it is meant to be used synchronously. However, it returns a promise which resolves when either a) the pushed value hits the channel buffer, or b) the pushed value is consumed via next.

So, for instance:

const chan = new Channel(async (push, stop) => {
  await push(1);
  console.log("1 consumed");
  await push(2);
  console.log("2 consumed");
  await push(3);
  console.log("3 consumed");
  stop();
});

await chan.next(); // logs "1 consumed"
await chan.next(); // logs "2 consumed"

If we don’t consume the next value, the channel is suspended at await push(3) forever, and "3 consumed" is never logged.

Alternatively:

const chan = new Channel(async (push, stop) => {
  await push(1);
  console.log("1 consumed");
  await push(2);
  console.log("2 consumed");
  await push(3);
  console.log("3 consumed");
  stop();
}, new FixedBuffer(100));
await chan.next();
// logs "1 consumed"
// logs "2 consumed"
// logs "3 consumed"

Because there is a buffer, the promises resolve immediately. This is the main difference between using a channel with a FixedBuffer versus using a channel without a buffer.

You do not have to await push calls. Reading the returned promise of the push function is entirely optional. The promise will never reject, and it’s mostly there for advanced use-cases where you want to make sure you’re not sending values faster than they’re being consumed.

[brainkim]

eslint go nuts because it doesn't want to allow floating promises

I am personally not a fan of the floating promises rule! I believe there are situations when you want to fire an async function and forget about it, and it should be okay. I think one of the rationales for this rule is that you should always handle unhandled promise rejections, but I believe you should simply use an environment where unhandled promise rejections crash the program.

[elderapo]

I see now. In that case, I think value returned from push should be created in a way it's easier to handle in a sane way. So far I've been writing push(1).catch(noop) where I wanted synchronously push but it felt wrong and counterintuitive.

Returning a promise when it's not always required to handle it like it can asynchronously throw is a bad idea IMO. I would suggest something like:

Option 1:

push(1); // { waitForPush: Promsie<void> }
await push(1).waitForPush();

Option 2:

push(1); // void
push(1, true); // { waitForPush: Promsie<void> }
await push(1, true).waitForPush();

What do you think?

[brainkim]

Are you using tslint? Personally I disagree with the no-floating-promises rule but there’s also a neat trick where you can suppress errors by using the void operator:

void push(1);

palantir/tslint#3056 (comment)

[elderapo]

Yes, I am using tslint. Haven't seen void promise construct before. Well, it works but looks a little bit confusing. I still believe that returning something that looks "chainable" would be a better approach since in most cases you don't really care.

[brainkim]

I don’t think I want to change the API of push just because a lint rule complains about it. Even if you change the API to something like push(1).waitForPush(), you haven’t really solved the problem of unused promises, because I can’t think of a way to defer the creation of the push promise until waitForPush is called. In other words, you’re really just changing the return value of push to void while still creating the promise and making it accessible elsewhere. Essentially, you’d be writing extra code to dodge a lint rule when the rule already has an accepted way to squash errors. I think the void call() construct is sufficient, but I also think that this rule isn’t that useful anyways.

[elderapo]

I disagree, this promise is a little bit weird because it is just a mechanism to ensure that something has been consumed. You said it never rejects.

So unless you need to ensure that pushed data has been consumed (rare use case) there is no reason to create and return that promise in the first place.

This can be easily done with an API like that:

push(data); // no promise created internally and returns void
push(data, false); // no promise created internally and returns void
push(data, true); // promise created internally and returned

[brainkim]

So unless you need to ensure that pushed data has been consumed (rare use case)

It’s not really a rare use-case. Take a look at channel combinators, @channel/timers or @channel/limiters, all the code uses the returned promise from push.

This can be easily done with an API like that:

I ran it through my head, and though I agree that it could be done with a boolean flag, I’m not sure it should be done for three reasons:

1. It’s really easy to forget the flag when you want the promise, leading to bugs.
2. I’m not sure I want to pass any more arguments to push, let alone a boolean flag. This sorta makes it hard to change the API later.
3. Conditionally typing push to return void or Promise is gonna be a pain in the butt.

Again, the only reason this is being asked for is the lint rule, and I fundamentally disagree with the rule’s premise, that you have to use every promise you create. According to that logic, the stop function should also be disallowed, insofar as it is a callable promise which you don’t always then/catch. At this point I want to say that this library and the API we chose is fundamentally incompatible with the no-floating-promises rule.

There is the void push() pattern if you want to keep the rule enabled but I don’t want to budge on my end.