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

http: remove pause()/resume() from parser bindings #22076

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 1, 2018

As far as I can tell, these are effectively no-ops
(http_parser_pause only sets the http_errno flag,
but that isn’t read anywhere), so removing them cleans
things up a tiny bit.

The corresponding regression test remains intact.

/cc @nodejs/http-parser

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

As far as I can tell, these are effectively no-ops
(`http_parser_pause` only sets the `http_errno` flag,
but that isn’t read anywhere), so removing them cleans
things up a tiny bit.

The corresponding regression test remains intact.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http Issues or PRs related to the http subsystem. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. labels Aug 1, 2018
@addaleax
Copy link
Member Author

addaleax commented Aug 2, 2018

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 2, 2018
@indutny
Copy link
Member

indutny commented Aug 3, 2018

They're not a no-op. The http_errno is checked on every http_parser_execute() run, and the parsing won't happen.

@addaleax
Copy link
Member Author

addaleax commented Aug 3, 2018

@indutny Thanks, that’s why I pinged the team… is there any behaviour that we could test against? It’s not quite ideal that all tests are passing without this, I’d say.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 3, 2018
@indutny
Copy link
Member

indutny commented Aug 4, 2018

I'm pretty sure that there's a testable effect of having pause/resume, not sure if it is desirable though 😛 It'd be great indeed if someone would write a test for it.

@mcollina
Copy link
Member

Why do we have to pause the parser as well? We are already pausing the underlining socket.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 6, 2018

Any progress here?

@Trott
Copy link
Member

Trott commented Nov 18, 2018

@addaleax Is this land-able after a rebase? Or is there something that has to happen here? (Write a test or something?)

@addaleax
Copy link
Member Author

@Trott Yes, this is more of a situation where we should test that these methods actually do something (and if it turns out that they actually don’t, we can still remove them). It’s on my backlog 😄

@Trott
Copy link
Member

Trott commented Nov 30, 2018

FWIW, needs a rebase.

@BridgeAR
Copy link
Member

Ping @addaleax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants