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

Using async/await in doto -> toPromise resolves to early #678

Open
ChimiDEV opened this issue Mar 26, 2019 · 7 comments
Open

Using async/await in doto -> toPromise resolves to early #678

ChimiDEV opened this issue Mar 26, 2019 · 7 comments

Comments

@ChimiDEV
Copy link

ChimiDEV commented Mar 26, 2019

My Problem is related to the version 3.0.0-beta.7
Tried to find some related issues but I didn't find anything close to it.

I have been using an AWS Lambda and the Highland libary with some functionality on top of it. (With the new use operator).

Everything worked like a charm until I started to use doto with an async function. When I try to consume the stream via toPromise(...), it seems that the Promise resolves before every asynchronous doto function resolved/finished. The resulting problem is, that the overall promise resolves too early, my Lambda considers his tasks as finished and aborts every ongoing request (as the process is ending) that is still pending.
However, that does not happen if I use the done operator with an callback (So no promises involved).

The question now is, as doto is only for side effects and async side effects are somehow hard to "track", could this be considered a bug or just a coincidence that has to be considered?

Thanks in advance.

Code Snippets

return ...
    .publishToIoTBroker(`${event.topic}/valid`) // <- My custom operator
    .toPromise(Promise);
function publishToIoTBroker(...) {
...
return this.doto(async data => {
...
await iotBroker.publish(...).promise()
});
}
@vqvu
Copy link
Collaborator

vqvu commented Mar 27, 2019

This is working as intended. doto is only meant for synchronous side effects or asynchronous side effects for which you don't care to wait.

If you're interested in waiting for the side effect to complete, use flatMap instead. There is no async version of doto.

return this.map(async data => {
      ...
      return data;
    })
    // coerces the promises back to streams and wait on them.
    .flatMap(_)

@ChimiDEV
Copy link
Author

Thanks a lot. I almost thought so. Is this the same for almost every operator? Like using async/await in each?
Maybe you can help me here:

d(myData)
  .map({...} => ({reqeustPromise, dataINeedToPropagate}))
  .each(async ({requestPromise, dataINeedToPropagate} => {
    const res = await reqeustPromise;
    ...
})

So my promises are mapped inside an object. Also each would consume my stream, I couldn't use

  1. Is this a valid option? Using async in each?
  2. If not, how could I say, I want the process to finish only if all each function calls are done?

Hope that is understandable. Thanks for your time.

@jaidetree
Copy link
Contributor

I don't anticipate that will work as intended, but I could be wrong.

Instead I would structure it like this:

const _ = require('highland');

// Used to generate random delays
function rand (max) {
  return Math.floor(Math.random() * Math.floor(max));
}

// Create a promise that resolves at a random delay of time
function promise (max) {
  return new Promise((resolve) => {
    setTimeout(() => resolve(Date.now()), rand(max));
  });
}


_([1, 2, 3])
  .map(async dataINeedToPropagate => {
    const promiseResult = await promise(1000);

    return { promiseResult, dataINeedToPropagate };
  })
  .flatMap(_)
  .each(({ promiseResult, dataINeedToPropagate }) => console.log(
      'received', promiseResult, dataINeedToPropagate
  ));

// =>
// received 1553699474556 1
// received 1553699475217 2
// received 1553699475405 3

@vqvu
Copy link
Collaborator

vqvu commented Mar 28, 2019

@eccentric-j is correct. There are no transform in highland that are async function-aware. So don't expect to be able to pass in an async function and have the stream wait for it.

How often do you you find yourself needing to use map with an async function? I actually wouldn't mind adding in an asyncMap(fn) that's equivalent to map(fn).flatMap(_), though it should be implemented directly using consume rather than stacking multiple transforms like this.

It'd be cool to have at least some limited interop with async functions.

@jaidetree
Copy link
Contributor

Would asyncMap be a preferred alternative to a more stream-centric approach?

_([1, 2, 3])
  .flatMap(dataINeedToPropagate => _(promise(1000))
    .map(promiseResult => ({promiseResult, dataINeedToPropagate }))
  )
  .each(({ promiseResult, dataINeedToPropagate }) => console.log(
      'received', promiseResult, dataINeedToPropagate
  ));

@vqvu
Copy link
Collaborator

vqvu commented Mar 28, 2019

As a library, I don't think we would have an opinion one way or the other.

However, personally, I think this is more readable alternative.

_([1, 2, 3])
    .asyncMap(async (dataINeedToPropagate => {
      const promiseResult = await promise(1000);
      return { promiseResult, dataINeedToPropagate };
    })

The async-await syntax is excellent, and we're not really dealing with a stream of multiple values, so I feel it's more awkward to convert into a stream just to deal with the asynchrony. Nested streams (like nested promises), are sometimes necessary, but not particularly easy to read.

Now, if the code were already split up into a separate function, like this

function doAsyncThing(data) {
  return _(promise(1000))
      .map(promiseResult => ({promiseResult, dataINeedToPropagate }));
}

_([1, 2, 3])
    .flatMap(doAsyncThing)
    ...

then I wouldn't rush to change doAyncThing to an async function even if asyncMap exists. There's less of a readability improvement in this case.

@jaidetree
Copy link
Contributor

I personally disagree about the readability as I prefer not having to switch paradigms and syntax. However, that is just my personal opinion and ultimately has nothing to do with this library.

I should be able to draft this operator near the beginning of April if no one beats me to it by then.

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

3 participants