Proposal: Require Streams WG to sign off on streams PRs

[mafintosh]

This came up in our latest Streams WG meeting. Recently there's been a couple of regressions after merging streams related PRs. Some of those were merged (or about to be merged) without a member of @nodejs/streams reviewing and signing off on it. The number of members in the WG is still relatively small (we are actively looking for more people to join) and the stream submodule has a complicated state machine and very few tests (we are working on improving this as well) so changes require very throughrough reviews.

Personally I've recently spent a lot of energy rushing to review PRs / going through commit history to avoid/catch unintended stream regressions being merged.

We would therefore like to propose to have it be a requirement that a @nodejs/streams member signs off on a stream related PR before it is merged until we can improve the current situation.

[bnoordhuis]

A 'yes' is going to hinge on mean and median review time. Getting sign-off from WG people shouldn't become a limiting factor in pull request velocity.

[mscdex]

I agree with Ben.

[calvinmetcalf]

@mscdex you've been doing great adding labels to the issues, if you pinged @nodejs/streams when you added a streams label, that would make it much easier for members of streams to review them as early as possible

@bnoordhuis that hopefully shouldn't happen but if it does something like "ping @nodejs/streams last call for input or in 2 business days i'm merging this)

[mcollina]

I see three problems here:

1. the streams wg has been doing very little in the last year
2. there are not enough people in the streams wg
3. streams are extremely complicated, and it's easy to damage stuff in userland (I know a thing or two about it)

So, to address 1, we are scheduling meeting every month. We are looking for more people to join the wg (hey folks, we need help!), and we spent half an hour trying to figure out a plan to avoid damaging things in userland.

We need automatic pinging on subsystem, this is no task a human can do. See nodejs/github-bot#1.

[mscdex]

I definitely think having a bot automatically ping would be better than relying on individual(s).

[Fishrock123]

For bot suggestions, please see the likes of nodejs/github-bot#1

Perhaps we can smoke-test mode streams users using citgm before landing streams commits? cc @thealphanerd

[jasnell]

Should we also be putting additional effort into improving / expanding the test coverage for streams to help catch these in CI?

[calvinmetcalf]

@jasnell yes that was what was exactly what we wanted to do, to hopefully avoid the need for this in the future.

[calvinmetcalf]

ok not sure where to announce this, but since I have you all here, for the purpose of smoke testing, setting READABLE_STREAM=disable as an environmental variable will now cause readable stream to just return the system version of node which should allow us to smoke test libraries that rely on readable stream

[jbergstroem]

I don't think a requirement is the way to go. I get sucked in every now and then through the build team and think that works great. If we could improve notifications/pings that would be my preferred outcome of this issue.

[orangemocha]

As the author of a recent PR that broke streams, I can attest to the fact that the code is hard to understand, and seems somewhat fragile. More tests would certainly help avoid regressions. Please also consider documenting the internals (e.g. the state machine) so that more people can understand and hopefully improve this code.

[MylesBorins]

I would like to suggest a Streams Summit

This could potentially be somewhat similar to the summits that @jasnell has organized in the past.

I for one would love to get more involved, but find that I don't exactly know where to start, and am a bit overwhelmed with the entire system.

Arranging a summit that can be part knowledge transfer and part project co-ordination could help move us in the right direction to making things more stable.

@nodejs/tsc is this something that ya'll might get behind?

[mcollina]

I'm really 👍 with the idea of a Stream Summit, not sure how the organization and logistic will work.

[Fishrock123]

I think @chrisdickinson has had the best suggestion in the past, something along the lines of documenting the entirety of the streams interface, as a base to start from.

Not sure if this should be done before, or as a Summit.

[mcollina]

@Fishrock123 I don't think it's possible to do it as a Summit. documenting the whole internals is a long process. I think we can kickstart that by doing short presentations and record them on the major areas, mainly to set a good base to discuss the future of those. It would be a good task for a new entry in the wg, mainly because we are all too biased to explain it in a way that's understandable (given the fact that we all learned it reading the code).

Maybe @nodejs/ctc can help setting this up? @rvagg?

[MylesBorins]

I'm open to helping lead the organization of an event like this. I had the chance to talk to @rvagg about it and he seemed positive about the possibility. I've opened a new issue up on the TSC repo to start the discussion more officially nodejs/TSC#93

[jasnell]

So the planning for having a new collaborators summit around the Interactive events is under way and it should include at least some time for the WG's to get together. On this particular issue, is there reason to keep this open?

Perhaps @Fishrock123 could use his experimental bot to perform automatic mentioning of the streams wg if the issue is tagged as stream?

[phillipj]

Perhaps @Fishrock123 could use his experimental bot to perform automatic mentioning of the streams wg if the issue is tagged as stream?

@jasnell absolutely, that's a trivial feature as soon as we're satisfied with the auto PR labelling test we're doing nowadays.

Refs #6247

[jasnell]

Closing this as I believe there's nothing else to do. Generally speaking, if it touches streams, someone from @nodejs/streams should sign off.