fix: pick ephemeral port once per agent

[ikonst]

Fixes spurious low level errors in the form of "socket hang up" when rapidly testing.

By creating a listening server just once per agent, we can allow callers to strategize by reusing the agent and avoiding (a) the setup overheard, and (b) spurious failure due to the ephemeral port being random on every agent construction (previously, on every request).

Fixes #844.

Checklist

• I have ensured my pull request is not behind the main or master branch of the original repository.
• I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
• I have written a commit message that passes commitlint linting.
• I have ensured that my code changes pass linting tests.
• I have ensured that my code changes pass unit tests.
• I have described my pull request and the reasons for code changes along with context if necessary.

[ikonst]

@titanism fyi

[titanism]

@ikonst can you elaborate more on this? is this a breaking change?

[titanism]

Doesn't it already listen on port 0 (random)?

supertest/lib/test.js

Line 48 in 2ae1c36

if (!addr) this._server = app.listen(0);

[ikonst]

can you elaborate more on this? is this a breaking change?

It shouldn't be, since nobody relies on the server's port being random on every call for the same agent instance. If you created an agent and reuse it, it's testing the same app instance. If even the app instance remains the same, there's really no need for the listening port to be randomized for every request.

Doesn't it already listen on port 0 (random)?

Yes, generally every random port it picks should work. It does seem that when rapidly making requests, if you pick a random port on every use of the agent, rather than once per agent, picking random ports quickly and sequentially makes the system run into edge cases.

[titanism]

I don't think that this is the right approach since it's starting the server before it is necessary. I don't think that this is the actual bug, since we're using .listen(0). Why is that not working already as is?

[ikonst]

Oh, I think I get what you're asking. You're doing

supertest/lib/test.js

Line 48 in 2ae1c36

if (!addr) this._server = app.listen(0);

which would indeed pick a random port. However, it's being done pretty late, so if you reuse the agent (i.e. make subsequent requests against the same agent), that .listen(0) would be done again and again, each time coming up with a different listening port.

By doing it early, I make it pick a port just once per agent instance.

[titanism]

Specifically, we need to know exactly what is causing this in particular, as I do not think it is a listen(0) issue:

Fixes spurious low level errors in the form of "socket hang up" when rapidly testing.

[ikonst]

Do you have ideas? I found the issue to consistently reproduce with:

const app = express();
app.get("/", (req, res) => res.send("OK"));
const superAgent = request(app);
for (let i = 0; i < 1000; i++) {
 await superAgent.get("/").expect(200);
}

and as documented in #844, pre-listening makes it consistently pass, i.e. this consistently passes:

 const app = express();
 app.get("/", (req, res) => res.send("OK"));
-const superAgent = request(app);
+const server = http.createServer(app).listen(0);
+const superAgent = request(server);

 for (let i = 0; i < 1000; i++) {
   await superAgent.get("/").expect(200);
 }

While this doesn't tell us why "socket hang up" would randomly occur, it gives us a strong hint that multiple cycles of picking an ephemeral port and listening on it make it more likely.

[ikonst]

One thing I notice is that Test's end relies on knowing the HTTP server's lifecycle, meaning that there's an expectation that the HTTP server will start for a specific request, and be closed after that request.

I do understand wanting to start it lazily, but what's the merit in closing in after every request to the same app?

BTW, could it be that the server.close() call is what occasionally throws the ConnResetException? It's a bit hard to figure out what causes it, since the error is thrown in event loop context.

[titanism]

Have you tested this on Node v18, Node v20, etc? Is this a node specific bug or thread issue?

[ikonst]

It reproduced on Node v18 and Node v23. I haven't tested it in-between. I'm as intrigued as you are, as to what causes those ECONNRESETs (that's the underlying system condition for what's manifested in Node as "socket hang up").

Would it be fair to make this change purely as an optimization? The real life use case I've stumbled on, is to test my app's rate limiter by issuing 500 requests rapidly and seeing whether request number 501 is rate limited.