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

Add Lwt_stream.iter_n #312

Merged
merged 1 commit into from
Feb 5, 2018
Merged

Add Lwt_stream.iter_n #312

merged 1 commit into from
Feb 5, 2018

Conversation

hcarty
Copy link
Contributor

@hcarty hcarty commented Jan 30, 2017

iter_n provides concurrent iteration over a stream with an upper bound
on the level of concurrency used.

`iter_n` provides concurrent iteration over a stream with an upper bound
on the level of concurrency used.
@hcarty
Copy link
Contributor Author

hcarty commented Jan 30, 2017

See #250 and #250 (comment) in the comment thread.

@aantron For what it's worth, I use this all over my code and it's absence from Lwt_stream is regularly by myself and at least one colleague. It makes it relatively simple to, for example, stream in a potentially unbounded list of URIs from a file with Lwt_io and submit them concurrently to a service with cohttp without accidentally committing a denial of service :-)

@agarwal
Copy link
Contributor

agarwal commented Jan 30, 2017

Async defines:

type how = [
| `Parallel
| `Sequential
| `Max_concurrent_jobs of int
]

and iter operations are defined like this:

val Deferred.List.iter : ?how:how -> 'a list -> f:('a -> unit Deferred.t) -> unit Deferred.t

Maybe it would be useful for here.

@hcarty
Copy link
Contributor Author

hcarty commented Jan 30, 2017

@agarwal Thanks - that seems promising. Could be worth adding for 3.0.0 or 4.0.0 to make this functionality more discoverable.

If we are going to have another 2.x release I'd like to see iter_n make it in. If not then it's maybe worth taking @agarwal's suggestion and reworking Lwt_stream and Lwt_list around a similar structure.

@hcarty
Copy link
Contributor Author

hcarty commented Feb 3, 2017

The CI failures look like they are unrelated to this PR.

@aantron
Copy link
Collaborator

aantron commented Feb 11, 2017

CI failures indeed unrelated to the PR. I restarted the failed builds, but the AppVeyor one might fail again.

This seems desirable, but I will have to delay taking a thorough look at it (including @agarwal's suggestion) for some time (weeks or months), due to other Lwt tasks in my queue.

@hcarty
Copy link
Contributor Author

hcarty commented Feb 11, 2017

If we take @agarwal's suggestion then it would ideally apply throughout at least Lwt_stream and Lwt_list.

@aantron
Copy link
Collaborator

aantron commented Feb 11, 2017

Yes. Indeed, in all cases, it would be nice to maintain some degree of consistency between Lwt_stream and Lwt_list, as well as the availability of the various _s, _p, and _n functions (if any).

@mfp
Copy link
Collaborator

mfp commented Feb 21, 2017 via email

@mfp
Copy link
Collaborator

mfp commented Feb 21, 2017 via email

@Drup
Copy link
Member

Drup commented Mar 2, 2017

@mfp Additionally, this looks very much like an lwt version of @c-cube's sequence.

@mfp
Copy link
Collaborator

mfp commented Mar 6, 2017 via email

@Drup
Copy link
Member

Drup commented Mar 10, 2017

@mfp Honestly, I have no idea. My gut feeling is "probably", but I never used any of them, really.

There is a clear distinction between Lwt_stream and "Sequence-like for lwt", the amount of control is not at all the same. I feel like these kind of combinators should wait a bit for the new iterators that might/should land in ocaml itself.

@aantron
Copy link
Collaborator

aantron commented Jan 23, 2018

I think we will merge this into Lwt_stream, and address design issues and #250 separately later. I don't think adding iter_n as a name to Lwt_stream is going to cause problems; we can always change the implementation as we make internal improvements, and if we change naming schemes, we can mark iter_n and other APIs deprecated as necessary.

@c-cube
Copy link
Collaborator

c-cube commented Jan 23, 2018

@Drup not sure the standard iterator type (if it ever gets merged) will be suitable for Lwt… A proper blocking iterator would have to return a 'a node Lwt.t instead of just 'a node

@aantron aantron merged commit ed19c3a into ocsigen:master Feb 5, 2018
@aantron
Copy link
Collaborator

aantron commented Feb 5, 2018

Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants