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

Assertion `parser->current_buffer_.IsEmpty()' Exception #34016

Closed
omkadiri opened this issue Jun 22, 2020 · 3 comments
Closed

Assertion `parser->current_buffer_.IsEmpty()' Exception #34016

omkadiri opened this issue Jun 22, 2020 · 3 comments
Labels
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.

Comments

@omkadiri
Copy link

Version: 12.0.0
Platform: UNIX AWS EC2

Processing lot of json file from S3 using worker_thread node module in an EC2(m4.xlarge)

Process step
(1)Got message for SQS
(2)Use message info to list bucket using listObjectV2
(3)Get each json from the list api call and process it(merge the data into one file).
(4)write back to S3.

This error only occurs when the number of json file increase to hundred of thousand

Exception message from run
/.nvm/versions/node/v12.0.0/bin/node[3884]: ../src/node_http_parser_impl.h:500:static void node::{anonymous}::Parser::Finish(const v8::FunctionCallbackInfov8::Value&): Assertion `parser->current_buffer_.IsEmpty()' failed.
1: 0x97a1c0 node::Abort() [/.nvm/versions/node/v12.0.0/bin/node]
2: 0x97a247 [/.nvm/versions/node/v12.0.0/bin/node]
3: 0x991492 [/.nvm/versions/node/v12.0.0/bin/node]
4: 0xb7a2b6 [/.nvm/versions/node/v12.0.0/bin/node]
5: 0xb7c1c9 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/.nvm/versions/node/v12.0.0/bin/node]
6: 0x1a5c1a2 [/.nvm/versions/node/v12.0.0/bin/node]

This file processing run successfully 99% of the time.

Node should catch this exception and print a detail message for why this exception occurs.

@jasnell jasnell added 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 Jun 22, 2020
@addaleax
Copy link
Member

Is there any chance you can verify that this also happens on the latest v12.x release? v12.0.0 was over a year ago, and lots of bug fixes have gone into it since then.

@omkadiri
Copy link
Author

I got the same exception running with version 12.18.1

/.nvm/versions/node/v12.18.1/bin/node[3921]: ../src/node_http_parser_impl.h:509:static void node::{anonymous}::Parser::Finish(const v8::FunctionCallbackInfov8::Value&): Assertion `parser->current_buffer_.IsEmpty()' failed.
1: 0xa08900 node::Abort() [/.nvm/versions/node/v12.18.1/bin/node]
2: 0xa0897e [/.nvm/versions/node/v12.18.1/bin/node]
3: 0xa20302 [/.nvm/versions/node/v12.18.1/bin/node]
4: 0xbeaee9 [/.nvm/versions/node/v12.18.1/bin/node]
5: 0xbeccd7 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [/.nvm/versions/node/v12.18.1/bin/node]
6: 0x13c9919 [/.nvm/versions/node/v12.18.1/bin/node]

@bnoordhuis
Copy link
Member

I haven't been able to reproduce but I can see the general shape of the bug. The call chain probably looks like this:

  • Parser::Execute() -> llhttp_execute() -> Parser::on_body() -> parserOnBody() -> ... -> parser.finish()

I think it should be safe to remove the CHECK and reset the current_buffer_ fields like this:

diff --git a/src/node_http_parser.cc b/src/node_http_parser.cc
index c7a3df8d06..f9729f3f51 100644
--- a/src/node_http_parser.cc
+++ b/src/node_http_parser.cc
@@ -514,7 +514,9 @@ class Parser : public AsyncWrap, public StreamListener {
     Parser* parser;
     ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());
 
-    CHECK(parser->current_buffer_.IsEmpty());
+    parser->current_buffer_.Clear();
+    parser->current_buffer_len_ = 0;
+    parser->current_buffer_data_ = nullptr;
     Local<Value> ret = parser->Execute(nullptr, 0);
 
     if (!ret.IsEmpty())

Opinions? Parser::Execute() performs the same reset after calling llhttp_execute().

@omkadiri If you want you can compile Node from source with that patch applied until this issue is resolved.

addaleax added a commit that referenced this issue Jul 14, 2020
Since the tests only crash on v12.x, this commit adds separate
regression tests.

Refs: #15102

PR-URL: #34250
Refs: #34016
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 14, 2020
Since the tests only crash on v12.x, this commit adds separate
regression tests.

Refs: #15102

PR-URL: #34250
Refs: #34016
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 16, 2020
Since the tests only crash on v12.x, this commit adds separate
regression tests.

Refs: #15102

PR-URL: #34250
Refs: #34016
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
cjihrig pushed a commit that referenced this issue Jul 23, 2020
Since the tests only crash on v12.x, this commit adds separate
regression tests.

Refs: #15102

PR-URL: #34250
Refs: #34016
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax added a commit that referenced this issue Sep 22, 2020
Since the tests only crash on v12.x, this commit adds separate
regression tests.

PR-URL: #34251
Refs: #15102
Refs: #34016
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

No branches or pull requests

4 participants