feature: add support for `StreamMap` to return on closed streams #4851

aviramha · 2022-07-21T07:30:39Z

Following #4847 - I think this is the best solution - I'm open to alternatives (especially as I'm not so good at generics, so there are probably improvements!)

Options I ruled out:

Wrapping the stream with a generic type that does this abstraction - requires wrapping each stream, then implementing all of it's functions if it has any extra besides the trait or needing to interact directly..
Different struct - "overkill" in terms of maintenance
Adding a function next_or_none - would require many abstractions to be added (next is only provided by StreamExt that relies on other functions, poll etc

Darksonn · 2022-07-21T09:11:45Z

Adding a third generic parameter for this seems a bit overkill. I would definitely prefer just providing a wrapper stream for this use-case instead.

aviramha · 2022-07-21T10:08:37Z

@Darksonn I understand what you're saying, but it feels funny as of right now Stream already provides a way of knowing it is closed (as it returns None when it ends). It's StreamMap implementation that decided to ignore it and wrapping the Stream itself feels like abuse

Darksonn · 2022-07-21T10:35:46Z

Whether or not the current design was a mistake doesn't really matter when it comes to designing how to add this feature. We need to pick the best design among the backwards compatible ones, and I don't agree that adding a third generic parameter is the best choice.

aviramha · 2022-07-21T12:49:04Z

What about changing the implementation based on a feature flag?

Darksonn · 2022-07-21T13:45:21Z

Enabling a feature flag must never change the behavior of code that compiles without it. This is because the crate is only compiled once, so if multiple things depend on tokio-stream and only some of them enable the feature flag, then they all get the feature flag.

Darksonn · 2022-08-10T13:16:03Z

Since changes to this PR have been requested, I am marking the PR as a draft. Please remove the draft marker when you would like me to review the PR again. If you no longer want to work on this PR, then please close the PR.

aviramha · 2022-08-10T13:26:25Z

Sorry, I hoped someone would have a better solution then the ones suggested so far. I'll change this PR to match your suggestion.

aviramha · 2022-08-23T10:42:35Z

Rewrote this as suggested (wrapper struct).

Darksonn

There are some minor issues that I have outlined below, but overall this looks really good! I'm happy to see that you included a test. :)

tokio-stream/src/wrappers/stream_close.rs

tokio-stream/src/wrappers.rs

tokio-stream/src/wrappers/stream_close.rs

Darksonn · 2022-08-23T11:03:09Z

tokio-stream/src/wrappers/stream_close.rs

+pin_project! {
+    /// Stream that returns None when the stream is exhausted. To be used with StreamMap to know when a stream ends.
+    #[must_use = "streams do nothing unless polled"]
+    pub struct StreamNotifyClose<S> {


The name is not great — notify normally means something different. But I can't think of anything better.

Totally agree. StreamClose implies nothing really.. would love suggestions.

tokio-stream/src/wrappers/stream_close.rs

aviramha · 2022-08-23T11:11:58Z

There are some minor issues that I have outlined below, but overall this looks really good! I'm happy to see that you included a test. :)

Thanks! Untested code is evil code ;)

Darksonn

I have some comments on your documentation.

tokio-stream/src/stream_close.rs

tokio-stream/src/stream_map.rs

tokio-stream/tests/stream_close.rs

Darksonn · 2022-08-24T11:41:00Z

There are some compilation failures in your documentation tests. You can see these on your local machine by running cargo test or cargo test --doc.

aviramha · 2022-08-24T12:23:23Z

cargo test --doc

Thanks for the feedback. I have submitted changes according to it.
Edit:
Just re-read the CONTRIBUTING.md and realized I'm squashing against the recommendation. Sorry about that.

…urns None when it ends, for use with StreamMap

tokio-stream/src/stream_close.rs

Co-authored-by: Alice Ryhl <[email protected]>

Darksonn · 2022-08-25T09:04:03Z

You will need to run rustfmt to make CI happy.

Other than that, my only concern about this is the naming. I still don't like using the word "notify" in the name for this.

aviramha · 2022-08-25T09:11:13Z

You will need to run rustfmt to make CI happy.

Other than that, my only concern about this is the naming. I still don't like using the word "notify" in the name for this.

I'm running cargo fmt --all and no changes?

Darksonn · 2022-08-25T09:14:36Z

The cargo fmt command is broken in the Tokio repo. The CONTRIBUTING.md file has the correct command.

aviramha · 2022-08-25T09:17:09Z

The cargo fmt command is broken in the Tokio repo. The CONTRIBUTING.md file has the correct command.

rustfmt --check --edition 2018 $(git ls-files '*.rs') doesn't show any output? maybe I'll just wait for the CI to show what's the issue and fix it manually.

Darksonn

Hi. I'm sorry that this got lost. However, it LGTM, so I'll go ahead and merge it. Thank you for the PR.

Darksonn added the A-tokio-stream Area: The tokio-stream crate label Jul 21, 2022

Darksonn marked this pull request as draft August 10, 2022 13:15

aviramha force-pushed the master branch from 7485de6 to e6b26c8 Compare August 23, 2022 10:40

aviramha marked this pull request as ready for review August 23, 2022 10:42

Darksonn reviewed Aug 23, 2022

View reviewed changes

aviramha force-pushed the master branch 3 times, most recently from 2515d3a to 7168215 Compare August 23, 2022 11:50

aviramha requested a review from Darksonn August 23, 2022 13:12

aviramha force-pushed the master branch from 7168215 to 4c9414e Compare August 23, 2022 13:14

Darksonn reviewed Aug 24, 2022

View reviewed changes

aviramha force-pushed the master branch from 52666fc to 4ca2fb2 Compare August 24, 2022 12:23

tokio-stream: add StreamNotifyClose to have a Stream wrapper that ret…

168ac23

…urns None when it ends, for use with StreamMap

aviramha force-pushed the master branch from 4ca2fb2 to 168ac23 Compare August 24, 2022 12:25

Darksonn reviewed Aug 24, 2022

View reviewed changes

tokio-stream/src/stream_close.rs Outdated Show resolved Hide resolved

tokio-stream/src/stream_close.rs Outdated Show resolved Hide resolved

aviramha and others added 2 commits August 24, 2022 17:26

Apply suggestions from code review

4a00c9b

Co-authored-by: Alice Ryhl <[email protected]>

cr

21ceacd

aviramha requested a review from Darksonn August 24, 2022 14:29

aviramha added 2 commits August 25, 2022 12:11

Merge branch 'master' into master

2eb7d83

Merge branch 'master' of https://github.com/aviramha/tokio

6f4ee0e

Merge branch 'master' into master

8079c4b

Darksonn approved these changes Apr 16, 2023

View reviewed changes

Merge branch 'master' into master

090babc

Darksonn enabled auto-merge (squash) April 16, 2023 14:38

Darksonn merged commit 9507f8b into tokio-rs:master Apr 16, 2023

folkertdev pushed a commit to folkertdev/tokio that referenced this pull request Apr 20, 2023

stream: add StreamNotifyClose (tokio-rs#4851)

7b1ae8a

folkertdev pushed a commit to folkertdev/tokio that referenced this pull request Apr 21, 2023

stream: add StreamNotifyClose (tokio-rs#4851)

12f4193

Darksonn mentioned this pull request Apr 25, 2023

chore: prepare tokio-stream v0.1.13 #5652

Merged

folkertdev pushed a commit to folkertdev/tokio that referenced this pull request May 19, 2023

stream: add StreamNotifyClose (tokio-rs#4851)

3179538

folkertdev pushed a commit to folkertdev/tokio that referenced this pull request May 23, 2023

stream: add StreamNotifyClose (tokio-rs#4851)

35b1c92

folkertdev pushed a commit to folkertdev/tokio that referenced this pull request May 29, 2023

stream: add StreamNotifyClose (tokio-rs#4851)

b8f1d0d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add support for `StreamMap` to return on closed streams #4851

feature: add support for `StreamMap` to return on closed streams #4851

aviramha commented Jul 21, 2022 •

edited

Loading

Darksonn commented Jul 21, 2022

aviramha commented Jul 21, 2022

Darksonn commented Jul 21, 2022

aviramha commented Jul 21, 2022

Darksonn commented Jul 21, 2022

Darksonn commented Aug 10, 2022

aviramha commented Aug 10, 2022

aviramha commented Aug 23, 2022

Darksonn left a comment

Darksonn Aug 23, 2022

aviramha Aug 23, 2022

aviramha commented Aug 23, 2022

Darksonn left a comment

Darksonn commented Aug 24, 2022

aviramha commented Aug 24, 2022 •

edited

Loading

Darksonn commented Aug 25, 2022

aviramha commented Aug 25, 2022

Darksonn commented Aug 25, 2022

aviramha commented Aug 25, 2022

Darksonn left a comment

feature: add support for StreamMap to return on closed streams #4851

feature: add support for StreamMap to return on closed streams #4851

Conversation

aviramha commented Jul 21, 2022 • edited Loading

Darksonn commented Jul 21, 2022

aviramha commented Jul 21, 2022

Darksonn commented Jul 21, 2022

aviramha commented Jul 21, 2022

Darksonn commented Jul 21, 2022

Darksonn commented Aug 10, 2022

aviramha commented Aug 10, 2022

aviramha commented Aug 23, 2022

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn Aug 23, 2022

Choose a reason for hiding this comment

aviramha Aug 23, 2022

Choose a reason for hiding this comment

aviramha commented Aug 23, 2022

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn commented Aug 24, 2022

aviramha commented Aug 24, 2022 • edited Loading

Darksonn commented Aug 25, 2022

aviramha commented Aug 25, 2022

Darksonn commented Aug 25, 2022

aviramha commented Aug 25, 2022

Darksonn left a comment

Choose a reason for hiding this comment

feature: add support for `StreamMap` to return on closed streams #4851

feature: add support for `StreamMap` to return on closed streams #4851

aviramha commented Jul 21, 2022 •

edited

Loading

aviramha commented Aug 24, 2022 •

edited

Loading