http: http_parser_llhttp regression

Based on test-http-response-status-message.js.

This will succeed with --http-parser legacy but fail otherwise.

'use strict';
const common = require('../common');
const http = require('http');
const net = require('net');

const server = net.createServer(function(con) {
  con.on('data', function(data) {
    con.write('HTTP/1.1 200\r\n\r\n');
    con.end();
  });
});

server.listen(0, function() {
  http.get({
    port: server.address().port
  }, function(res) {
    res.on('aborted', common.mustNotCall());
    res.on('end', common.mustCall());
    res.resume();
  });
});

I believe the newer parser is more correct. From RFC 7230:

status-line = HTTP-version SP status-code SP reason-phrase CRLF
reason-phrase = *( HTAB / SP / VCHAR / obs-text )

So you can have an empty reason-phrase, but you still need at least the space before it.

@mscdex

@mscdex so I guess the tests need to be updated?

con.write('HTTP/1.1 200 OK\r\n\r\n'); will also fail

An issue should be posted to https://github.com/nodejs/llhttp/issues then.

This should fix it: https://github.com/nodejs/llhttp/pull/26/files . Sorry if it caused a disruption!

The fix has been landed and released. Here is a PR to update llhttp: #28154

Navigation Menu

Repository navigation

http: http_parser_llhttp regression #28137

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

http: http_parser_llhttp regression #28137

Description

Activity

mscdex commented on Jun 8, 2019

ronag commented on Jun 8, 2019

ronag commented on Jun 8, 2019

mscdex commented on Jun 8, 2019

indutny commented on Jun 9, 2019

indutny commented on Jun 10, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions