chore: disable 'test-http-content-length.js` test

[bartlomieju]

…d of Transfer-Encoding: chunked (#20127)"

This reverts commit c2547ba.

After merging that PR there's quite a lot of flakiness in Node compat tests that
fail like so:

Node.js compatibility ... Node.js compatibility "parallel/test-http-content-length.js" => ./cli/tests/node_compat/test.ts:58:11
error: AssertionError: Failed assertion: "parallel/test-http-content-length.js" failed:

error: Uncaught (in promise) TypeError: error sending request for url (https://localhost:50029/multiple-writes): connection error: Connection reset by peer (os error 54)
    at async Promise.all (index 0)
    at async node:http:432:26


You can repeat only this test with the command:

  ./target/debug/deno test -A cli/tests/node_compat/test.ts -- parallel/test-http-content-length.js

    throw new AssertionError(msg);
          ^
    at assert (file:https:///Users/runner/work/deno/deno/test_util/std/testing/asserts.ts:141:11)
    at fail (file:https:///Users/runner/work/deno/deno/test_util/std/testing/asserts.ts:602:3)
    at fn (file:https:///Users/runner/work/deno/deno/cli/tests/node_compat/test.ts:122:9)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async innerWrapped (ext:cli/40_testing.js:483:5)
    at async Object.outerWrapped [as fn] (ext:cli/40_testing.js:428:14)
    at async TestContext.step (ext:cli/40_testing.js:1241:22)
    at async runTest (file:https:///Users/runner/work/deno/deno/cli/tests/node_compat/test.ts:58:3)
    at async file:https:///Users/runner/work/deno/deno/test_util/std/async/pool.ts:76:11

 FAILURES 

Node.js compatibility ... Node.js compatibility "parallel/test-http-content-length.js" => ./cli/tests/node_compat/test.ts:58:11

CC @osddeitf

[osddeitf]

The problem may lie in the node-compat test I added.
The test was problematic when I first added it from node test suite, it couldn't fail even if there's AssertionError, so I tried to modify it.
If the test fails often and affect the automation test, please consider revert only the test.

[osddeitf]

The previous behavior of Deno node:http is to send request when the class ClientRequest is constructed.
What I modified is to delay the request until the first body chunk get sent, or when request.end().
That also fixes the methods setHeader(), flushHeader() and removeHeader().

[bartlomieju]

The problem may lie in the node-compat test I added. The test was problematic when I first added it from node test suite, it couldn't fail even if there's AssertionError, so I tried to modify it. If the test fails often and affect the automation test, please consider revert only the test.

@osddeitf could you point me to the relevant part that should fail but didn't? Maybe there's another bug hiding there that I could fix.

[osddeitf]

This is my fix attempt:

• 724c966: assert only relevant headers.
• 1a98037: close http server when there's an AssertionError (if the server still running, the test can't stop).
• aca7b73: the test couldn't fail so I store the AssertionError and throw when the server is closed.

I'll pull the latest code and try to rerun the test again, before, it worked totally normal for me (no flakiness), it may also because of other changes to Node compatibility layer.

[bartlomieju]

@osddeitf ideally we'd just use test file from Node without any changes, if it fails because of discrepancies in sent headers we can remove unnecessary headers from being sent.

[osddeitf]

I know, that's why I changed that in the first commit mentioned above. The test passed as I expected.
But to make sure I tried to switch to earlier version of Deno to test if it would fail.
And instead of fail, the test do print the AssertionError, but the http server is not closed because countdown never reached 0, the test just stuck there instead of stop.
Hence the second commit.

[bartlomieju]

Alright, let's then work on fixing these assertion errors!

[osddeitf]

Thank you! We still have some time till v1.37, I'll do my part too when I have some free time.

[bartlomieju]

Thank you! We still have some time till v1.37, I'll do my part too when I have some free time.

Sounds good, I will then disable the test for now as it is causing flakes on CI

[mmastrac]

LGTM

[osddeitf]

@bartlomieju let me report on what I found (again, sorry if it's too long):

• The error message indicates a Promise.all() rejection with index 0, I was able to pinpoint the error at ClientRequest.end() in node:http:

    (async () => {
      try {
        const [res, _] = await Promise.all([
          core.opAsync("op_fetch_send", this._req.requestRid),

  Note that the promise is awaited, and it is wrapped inside an async IIFE.

• Deno implementation of http.Server:

  • server.listen(): http.ts#L1621.
  • server.close(): http.ts#L1679.

  Note that the server is abruptly closed.

• Node implementation of http.Server (kinda complicated):

  • http.Server inherit net.Server
  • Calling server.listen() -> server.listenInCluster() -> server._listen2() -> onconnection() -> increase connections count.
  • Calling server.close() -> server._emitCloseIfDrained() -> don't close when there are connection(s).

  Note that the server gracefully shutdown, waiting for connections to close.

• Error message Connection reset by peer (os error 54), indicates that the request did get to the server, but the server suddenly closed.

So I surmise that inside the test, the server get closed (when countdown reached 0) before res.end() complete, and as a result, the request get closed abrubtly.

[osddeitf]

In Nodejs docs server.close([callback]):

Stops the server from accepting new connections. See net.Server.close().

So I think this is indeed an issue of Server.close() in Deno implementation.

[bartlomieju]

@osddeitf no worries. Nice investigation. Let's wait for #20387 to land and then it should be an easy fix 👌