http: fix llhttp_finish semantics and add tests

Although implemented in a compatible way `llhttp_finish()` was never
properly tested until now. Unsurprisingly, there was an incorrect
behavior hidden in there.

Fix: #25
PR-URL: #26
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

package.json

@@ -39,7 +39,7 @@
     "@types/mocha": "^5.2.5",
     "@types/node": "^10.12.9",
     "llparse-dot": "^1.0.1",
-    "llparse-test-fixture": "^3.1.0",
+    "llparse-test-fixture": "^3.3.0",
     "mdgator": "^1.1.2",
     "mocha": "^5.2.0",
     "ts-node": "^7.0.1",

src/llhttp/http.ts

@@ -582,8 +582,8 @@ export class HTTP {
           span.body.end(n('message_done')))));
 
     n('body_identity_eof')
-      .otherwise(span.body.start(
-        this.update('finish', FINISH.SAFE_WITH_CB, 'eof')));
+      .otherwise(
+        this.update('finish', FINISH.SAFE_WITH_CB, span.body.start(n('eof'))));
 
     // Just read everything until EOF
     n('eof')
@@ -671,8 +671,9 @@ export class HTTP {
     // Check if we'd like to keep-alive
     if (this.mode === 'strict') {
       n('cleanup')
-        .otherwise(p.invoke(callback.afterMessageComplete, this.mode === 'strict' ?
-          { 1: n('restart') } : {}, n('closed')));
+        .otherwise(p.invoke(callback.afterMessageComplete, {
+          1: n('restart'),
+        }, this.update('finish', FINISH.SAFE, n('closed'))));
     } else {
       n('cleanup')
         .otherwise(p.invoke(callback.afterMessageComplete, n('restart')));
@@ -684,7 +685,7 @@ export class HTTP {
         'Data after `Connection: close`'));
 
     n('restart')
-      .otherwise(n('start'));
+      .otherwise(this.update('finish', FINISH.SAFE, n('start')));
   }
 
   private node<T extends Node>(name: string | T): T {

test/fixtures/extra.c

@@ -1,3 +1,5 @@
+#include <stdlib.h>
+
 #include "fixture.h"
 
 int llhttp__on_url(llparse_t* s, const char* p, const char* endp) {
@@ -55,6 +57,11 @@ void llhttp__test_init_response(llparse_t* s) {
 }
 
 
+void llhttp__test_finish(llparse_t* s) {
+  llparse__print(NULL, NULL, "finish=%d", s->finish);
+}
+
+
 int llhttp__on_status(llparse_t* s, const char* p, const char* endp) {
   if (llparse__in_bench)
     return 0;

test/fixtures/index.ts

@@ -8,7 +8,8 @@ import * as path from 'path';
 
 import * as llhttp from '../../src/llhttp';
 
-export type TestType = 'request' | 'response' | 'none' | 'url';
+export type TestType = 'request' | 'response' | 'request-finish' |
+  'response-finish' | 'none' | 'url';
 
 export { FixtureResult };
 
@@ -57,6 +58,13 @@ export function build(llparse: LLParse, node: any, outFile: string,
   const extra = options.extra === undefined ? [] : options.extra.slice();
   if (ty === 'request' || ty === 'response') {
     extra.push(`-DLLPARSE__TEST_INIT=llhttp__test_init_${ty}`);
+  } else if (ty === 'request-finish' || ty === 'response-finish') {
+    if (ty === 'request-finish') {
+      extra.push('-DLLPARSE__TEST_INIT=llhttp__test_init_request');
+    } else {
+      extra.push('-DLLPARSE__TEST_INIT=llhttp__test_init_response');
+    }
+    extra.push('-DLLPARSE__TEST_FINISH=llhttp__test_finish');
   }
 
   return fixtures.build(artifacts, outFile, Object.assign(options, {

test/md-test.ts

@@ -75,16 +75,20 @@ interface IFixtureMap {
 
 const http: IFixtureMap = {
   loose: {
-    none: buildMode('loose', 'none'),
-    request: buildMode('loose', 'request'),
-    response: buildMode('loose', 'response'),
-    url: buildMode('loose', 'url'),
+    'none': buildMode('loose', 'none'),
+    'request': buildMode('loose', 'request'),
+    'request-finish': buildMode('loose', 'request-finish'),
+    'response': buildMode('loose', 'response'),
+    'response-finish': buildMode('loose', 'response-finish'),
+    'url': buildMode('loose', 'url'),
   },
   strict: {
-    none: buildMode('strict', 'none'),
-    request: buildMode('strict', 'request'),
-    response: buildMode('strict', 'response'),
-    url: buildMode('strict', 'url'),
+    'none': buildMode('strict', 'none'),
+    'request': buildMode('strict', 'request'),
+    'request-finish': buildMode('strict', 'request-finish'),
+    'response': buildMode('strict', 'response'),
+    'response-finish': buildMode('strict', 'response-finish'),
+    'url': buildMode('strict', 'url'),
   },
 };
 
@@ -141,6 +145,10 @@ function run(name: string): void {
           types = [ 'request' ];
         } else if (meta.type === 'response-only') {
           types = [ 'response' ];
+        } else if (meta.type === 'request-finish') {
+          types = [ 'request-finish' ];
+        } else if (meta.type === 'response-finish') {
+          types = [ 'response-finish' ];
         } else {
           throw new Error(`Invalid value of \`type\` metadata: "${meta.type}"`);
         }
@@ -229,11 +237,13 @@ run('request/connection');
 run('request/content-length');
 run('request/transfer-encoding');
 run('request/invalid');
+run('request/finish');
 
 run('response/sample');
 run('response/connection');
 run('response/content-length');
 run('response/transfer-encoding');
 run('response/invalid');
+run('response/finish');
 
 run('url');

test/request/finish.md

@@ -0,0 +1,53 @@
+Finish
+======
+
+Those tests check the return codes and the behavior of `llhttp_finish()` C API.
+
+## It should be safe to finish after GET request
+
+<!-- meta={"type": "request-finish"} -->
+```http
+GET / HTTP/1.1
+
+
+```
+
+```log
+off=0 message begin
+off=4 len=1 span[url]="/"
+off=18 headers complete method=1 v=1/1 flags=0 content_length=0
+off=18 message complete
+off=NULL finish=0
+```
+
+## It should be unsafe to finish after incomplete PUT request
+
+<!-- meta={"type": "request-finish"} -->
+```http
+PUT / HTTP/1.1
+Content-Length: 100
+
+```
+
+```log
+off=0 message begin
+off=4 len=1 span[url]="/"
+off=16 len=14 span[header_field]="Content-Length"
+off=32 len=3 span[header_value]="100"
+off=NULL finish=2
+```
+
+## It should be unsafe to finish inside of the header
+
+<!-- meta={"type": "request-finish"} -->
+```http
+PUT / HTTP/1.1
+Content-Leng
+```
+
+```log
+off=0 message begin
+off=4 len=1 span[url]="/"
+off=16 len=12 span[header_field]="Content-Leng"
+off=NULL finish=2
+```

test/response/finish.md

@@ -0,0 +1,20 @@
+Finish
+======
+
+Those tests check the return codes and the behavior of `llhttp_finish()` C API.
+
+## It should be safe to finish with cb after empty response
+
+<!-- meta={"type": "response-finish"} -->
+```http
+HTTP/1.1 200 OK
+
+
+```
+
+```log
+off=0 message begin
+off=13 len=2 span[status]="OK"
+off=19 headers complete status=200 v=1/1 flags=0 content_length=0
+off=NULL finish=1
+```