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

Awaiting async function in async iterator #77

Merged
merged 6 commits into from Nov 19, 2020

Conversation

George-Payne
Copy link
Member

  • Allow awaiting within async iterators
    • Queue events and pull as needed
    • If no events in queue, wait for next event
    • Move to seperate SubscriptionItorator and return that in async iteration (for both subscription types)
  • Add same test case for persistent subscriptions

thomastoye and others added 3 commits November 13, 2020 21:30
- Queue events and pull as needed
- If no events in queue, wait for next event
- Move to seperate SubscriptionItorator and return that in async iteration (for both subscription types)
@George-Payne George-Payne self-assigned this Nov 16, 2020
@George-Payne George-Payne added the kind/bug Issues which are a software defect label Nov 16, 2020
@thomastoye
Copy link
Contributor

Won't using a queue like this run the risk of running out of memory if the stream that is subscribed to is large and the async function takes long? I'm not seeing any backpressure. Would it be possible to access a readable stream from a subscription?

- allow subscriptions to be paused and resumed
- test that events aren't skipped or duplicated when stream is paused and resumed
@George-Payne
Copy link
Member Author

You're quite right, I've capped the length of the queue to be 10 by pausing and resuming the subscription.

@George-Payne
Copy link
Member Author

Would it be possible to access a readable stream from a subscription?

Looks like the correct way to do this, but I'll need to do more reading before I go about implementing it. I've opened an issue (#78) to track it.

@thomastoye
Copy link
Contributor

I'm not sure if the GPRC stream are Node streams as well, but if they are, you may be able to simplify this, since Node readable streams are async iterables/have a Symbol.asyncIterable property.

@thomastoye
Copy link
Contributor

Dropping by to let you know that this fixed my issue 👍

@George-Payne
Copy link
Member Author

I'm not sure if the GPRC stream are Node streams as well, but if they are, you may be able to simplify this, since Node readable streams are async iterables/have a Symbol.asyncIterable property.

I've moved it over to using streams, need to decide if / how we want to expose the stream directly. It might be that I replace Subscription with a stream, but it needs more thought. I'll keep this tracked in #78

@jageall jageall requested a review from YoEight November 18, 2020 12:20
@hayley-jean hayley-jean merged commit 56fcf40 into master Nov 19, 2020
@hayley-jean hayley-jean deleted the awaiting-async-function-in-async-iterator branch November 19, 2020 12:17
@thomastoye
Copy link
Contributor

🎉

@George-Payne / @hayley-jean Could you publish a new package version?

@George-Payne
Copy link
Member Author

@thomastoye Just published 0.0.0-alpha.8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Issues which are a software defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants