Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make unbounded channels size warning exact (part 2) #13504

Merged
merged 9 commits into from
Mar 7, 2023
Merged

Make unbounded channels size warning exact (part 2) #13504

merged 9 commits into from
Mar 7, 2023

Conversation

dmitry-markin
Copy link
Contributor

@dmitry-markin dmitry-markin commented Mar 1, 2023 •
edited
Loading

This is a second part of #13490 that switches from futures-channel to async-channel in utils/mpsc.

CC @nazar-pc

@dmitry-markin dmitry-markin added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Mar 1, 2023
bkchr
bkchr reviewed Mar 1, 2023
View reviewed changes
client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
@koute
Copy link
Contributor

koute commented Mar 2, 2023

Hmm... reading this makes me think, do we actually need to support the not(feature = "metered")? Couldn't this channel just always be metered?

koute
koute reviewed Mar 2, 2023
View reviewed changes
client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
melekes
melekes approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
@dmitry-markin dmitry-markin requested a review from koute March 3, 2023 08:24
@dmitry-markin
Copy link
Contributor Author

Hmm... reading this makes me think, do we actually need to support the not(feature = "metered")? Couldn't this channel just always be metered?

Currently UNBOUNDED_CHANNELS_COUNTER is behind #[cfg(feature = "metered")], so this would need to be changed too.

Otherwise, I don't know if this metered feature makes sense.

@dmitry-markin dmitry-markin requested a review from bkchr March 6, 2023 08:08
@bkchr
Copy link
Member

bkchr commented Mar 6, 2023

Yeah, let's remove the feature flag and let it be metered always!

@dmitry-markin
Copy link
Contributor Author

Yeah, let's remove the feature flag and let it be metered always!

Done!

koute
koute reviewed Mar 6, 2023
View reviewed changes
client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
client/utils/src/mpsc.rs Outdated Show resolved Hide resolved
koute
koute approved these changes Mar 6, 2023
View reviewed changes
client/utils/src/mpsc.rs Show resolved Hide resolved
@dmitry-markin dmitry-markin merged commit 2aa2b4e into master Mar 7, 2023
@dmitry-markin dmitry-markin deleted the dm-async-channel-utils branch March 7, 2023 07:57
@kiltbot kiltbot mentioned this pull request Apr 3, 2023
Closed
@NingLin-P NingLin-P mentioned this pull request Apr 6, 2023
Closed
2 tasks
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
@dmitry-markin @ukint-vs
Make unbounded channels size warning exact (part 2) (paritytech#13504)
5352eca
@koushiro
Copy link
Contributor

@dmitry-markin so why you replace futures-channel with async-channel, I don't see the good reason in this PR description.
And this change is a break change of TracingUnboundedSender.

@dmitry-markin
Copy link
Contributor Author

@dmitry-markin so why you replace futures-channel with async-channel, I don't see the good reason in this PR description. And this change is a break change of TracingUnboundedSender.

We introduced the diagnostics of channel clogging in #12971 that fires an error if the queue grows above the limit. Originally we thought of limits in the order of 100's or 1000's, so didn't pay attention to off-by-one errors. It was later requested to make the queue size trigger exact. This is what this PR did (and we needed the channel that provides len() functionality out of the box for this, see #13117 for detailed discussion).

@kacperzuk-neti kacperzuk-neti mentioned this pull request Jun 23, 2023
Merged
15 tasks
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
@dmitry-markin @nathanwhit
Make unbounded channels size warning exact (part 2) (paritytech#13504)
4d264ff
@dmitry-markin dmitry-markin mentioned this pull request Sep 11, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@koute koute koute approved these changes

@melekes melekes melekes approved these changes

@andresilva andresilva Awaiting requested review from andresilva

@bkchr bkchr Awaiting requested review from bkchr

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
SDK Node
Status: done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dmitry-markin @koute @bkchr @koushiro @melekes