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

stream: manual destroy ServerResponse on pipeline #41137

Conversation

RafaelGSS
Copy link
Member

@RafaelGSS RafaelGSS commented Dec 10, 2021

Change the behavior to not destroy the ServerResponse stream on stream.pipeline when
an error happens.

Address: #26311


I feel that #38262 was a similar issue that was solved differently, however, I do not find a better way to solve it so far. I'm completely open to changing the current approach to a non-breaking one.

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Dec 10, 2021
@RafaelGSS RafaelGSS force-pushed the fix/pipeline-incoming-request-destroy branch 2 times, most recently from 6ef08bd to e13d9ef Compare December 10, 2021 16:35
@RafaelGSS RafaelGSS force-pushed the fix/pipeline-incoming-request-destroy branch from e13d9ef to 0a819b9 Compare December 10, 2021 17:55
@ronag
Copy link
Member

ronag commented Dec 10, 2021

I think you mean ServerResponse which is not an IncomingMessage.

@ronag
Copy link
Member

ronag commented Dec 10, 2021

I'm not sure I agree with this. The pattern in #26311 is not something I'd recommend.

@ronag
Copy link
Member

ronag commented Dec 10, 2021

I think it's better to add an option to pipeline to not destroy the last stream.

@RafaelGSS
Copy link
Member Author

I think it's better to add an option to pipeline to not destroy the last stream.

It's not exactly what #34133 was doing?

@RafaelGSS RafaelGSS changed the title stream: manual destroy IncomingRequest on pipeline stream: manual destroy ServerResponse on pipeline Dec 10, 2021
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

You are changing the behavior of server response instead of making it an explicit option on pipeline.

@RafaelGSS
Copy link
Member Author

You are changing the behavior of server response instead of making it an explicit option on pipeline.

@ronag I don't know if I get it. I mean, the #34133 was created to solve the same issue using the "explicit option" approach, however, the feature was considered not needed (sorry if I'm missing something).

This PR uses a different approach to solve the same issue avoiding an explicit option adding more complexity to the end-user, but looks not acceptable either (and I completely understand your point of view).

In this case, shall we re-open the other PR and close this one?

@RafaelGSS
Copy link
Member Author

If none of those options are available, perhaps a note in the documentation noticing those edge cases using ServerResponse inside pipeline would be necessary, what do you think?

@RafaelGSS
Copy link
Member Author

RafaelGSS commented Jan 31, 2022

Hey @ronag, friendly ping. Maybe should we update the documentation by adding this edge case?

@ronag
Copy link
Member

ronag commented Jan 31, 2022

I'm not quite sure what edge cases you are referring to but adding documentations is usually a good idea. I'm not entirely up to speed here anymore.

@RafaelGSS
Copy link
Member Author

Closing in favor of #41796

@RafaelGSS RafaelGSS closed this Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants