Skip to content

Commit

Permalink
http: simplify parser lifetime tracking
Browse files Browse the repository at this point in the history
Instead of providing a separate class for keeping the
parser alive during its own call back, just delay a
possible `.close()` call until the stack has cleared
completely.

PR-URL: #18135
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
addaleax authored and rvagg committed Aug 16, 2018
1 parent 59f1330 commit e094275
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 24 deletions.
5 changes: 4 additions & 1 deletion lib/_http_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ var parsers = new FreeList('parsers', 1000, function() {
return parser;
});

function closeParserInstance(parser) { parser.close(); }

// Free the parser and also break any links that it
// might have to any other things.
Expand All @@ -208,7 +209,9 @@ function freeParser(parser, req, socket) {
parser.outgoing = null;
parser[kOnExecute] = null;
if (parsers.free(parser) === false) {
parser.close();
// Make sure the parser's stack has unwound before deleting the
// corresponding C++ object through .close().
setImmediate(closeParserInstance, parser);
} else {
// Since the Parser destructor isn't going to run the destroy() callbacks
// it needs to be triggered manually.
Expand Down
24 changes: 1 addition & 23 deletions src/node_http_parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -378,8 +378,7 @@ class Parser : public AsyncWrap {
Parser* parser;
ASSIGN_OR_RETURN_UNWRAP(&parser, args.Holder());

if (--parser->refcount_ == 0)
delete parser;
delete parser;
}


Expand Down Expand Up @@ -545,22 +544,6 @@ class Parser : public AsyncWrap {
}

protected:
class ScopedRetainParser {
public:
explicit ScopedRetainParser(Parser* p) : p_(p) {
CHECK_GT(p_->refcount_, 0);
p_->refcount_++;
}

~ScopedRetainParser() {
if (0 == --p_->refcount_)
delete p_;
}

private:
Parser* const p_;
};

static const size_t kAllocBufferSize = 64 * 1024;

static void OnAllocImpl(size_t suggested_size, uv_buf_t* buf, void* ctx) {
Expand Down Expand Up @@ -597,8 +580,6 @@ class Parser : public AsyncWrap {
if (nread == 0)
return;

ScopedRetainParser retain(parser);

parser->current_buffer_.Clear();
Local<Value> ret = parser->Execute(buf->base, nread);

Expand Down Expand Up @@ -736,7 +717,6 @@ class Parser : public AsyncWrap {
char* current_buffer_data_;
StreamResource::Callback<StreamResource::AllocCb> prev_alloc_cb_;
StreamResource::Callback<StreamResource::ReadCb> prev_read_cb_;
int refcount_ = 1;

// These are helper functions for filling `http_parser_settings`, which turn
// a member function of Parser into a C-style HTTP parser callback.
Expand All @@ -753,8 +733,6 @@ class Parser : public AsyncWrap {
typedef int (Parser::*DataCall)(const char* at, size_t length);

static const struct http_parser_settings settings;

friend class ScopedRetainParser;
};

const struct http_parser_settings Parser::settings = {
Expand Down

0 comments on commit e094275

Please sign in to comment.