Skip to content

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
Copy link
Collaborator

@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