Platform-node http client not handling ECONNRESET socket errors

leonitousconforti commented 9 months ago

I am working with the platform-node implementation of the effect http client and am experiencing an issue where when the remote server closes/destroys the socket (and an ECONNRESET error is thrown) the http request hangs forever. I've created a minimal reproducible example which you can find below. I think this behavior is a bug and would be willing to submit a PR but I am still quite slow at debugging this library so any help/pointers would be appreciated. If you don't think this is a bug, could you explain why this is desired and how I might be able to avoid it?

import * as NodeHttp from "@effect/platform-node/HttpClient";
import { Effect } from "effect";
import http from "node:http";

// Dummy http server that forcefully close the connection with an ECONNRESET error every time
http.createServer((request) => {
    request.socket.destroy();
}).listen(5000);

// Node.js request correctly handles this
const testRequest = (url: string) =>
    new Promise((resolve, reject) => {
        const request = http.request(url, (response) => {
            const data: string[] = [];
            response.on("data", (chunk) => {
                data.push(chunk);
            });
            response.on("end", () => {
                resolve(data.join(""));
            });
            response.on("error", (error) => {
                reject(error);
            });
        });
        request.end();
        request.on("error", (error) => {
            reject(error);
        });
    });

// <!doctype html>
// <html>
// <head>
//     <title>Example Domain</title>
// ...
await testRequest("http://example.com").then(console.log);

// Error: socket hang up
//     at connResetException (node:internal/errors:787:14)
//     at Socket.socketOnEnd (node:_http_client:519:23)
//     at Socket.emit (node:events:526:35)
//     at endReadableNT (node:internal/streams/readable:1589:12)
//     at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
//   code: 'ECONNRESET'
// }
await testRequest("http://localhost:5000/_ping").catch(console.error);

// HANGS FOREVER!!
// If I add a console.log(error) before the resume call on this line: https://github.com/Effect-TS/platform/blob/0f19070a9fc9d2f3faa7c0191c47af65ccfcfece/packages/platform-node/src/internal/http/nodeClient.ts#L157
// I can see this error message
// Error: socket hang up
//     at connResetException (node:internal/errors:787:14)
//     at Socket.socketOnEnd (node:_http_client:519:23)
//     at Socket.emit (node:events:526:35)
//     at endReadableNT (node:internal/streams/readable:1589:12)
//     at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
//   code: 'ECONNRESET'
// }
// But it never bubbles up?
// So this effect hangs forever
const result = await Effect.gen(function* (_: Effect.Adapter) {
    const client: NodeHttp.client.Client.Default = yield* _(NodeHttp.nodeClient.make);

    const response: string = yield* _(
        NodeHttp.request
            .make("GET")("/")
            .pipe(NodeHttp.request.prependUrl("http://localhost:5000"))
            .pipe(client.pipe(NodeHttp.client.filterStatusOk))
            .pipe(Effect.flatMap((response) => response.text))
    );

    return response;
})
    .pipe(Effect.orDie)
    .pipe(Effect.provide(NodeHttp.nodeClient.agentLayer))
    .pipe(Effect.runPromise);

console.log(result);

"dependencies": {
    "effect": "2.0.0-next.59",
    "@effect/platform-node": "0.34.0"
}

oh and node v20.10.0

leonitousconforti commented 9 months ago

One possible idea I have right now is that if Effect.async behaves similar to Promises in JS, i.e once they are "settled" they can't change, then in this block: https://github.com/Effect-TS/platform/blob/0f19070a9fc9d2f3faa7c0191c47af65ccfcfece/packages/platform-node/src/internal/http/nodeClient.ts#L154-L172

since the finish event is emitted before the error event in this case, the effect is "settled" with a success instead of the failure? I just tested this by commenting out the:

  nodeRequest.on("finish", () => {
    console.log("finish")
    resume(Effect.unit);
  });

and the error does indeed bubble up now. No idea why the finish event is emitted first in this case

leonitousconforti commented 9 months ago

Ok I think I can follow what is going on now,

https://github.com/Effect-TS/platform/blob/0f19070a9fc9d2f3faa7c0191c47af65ccfcfece/packages/platform-node/src/internal/http/nodeClient.ts#L81-L87

calls sendBody and waitForResponse. Since this request has no body, sendBody goes straight to waitForFinish which I infer means something like "wait for the request to finish." Reading what the docs for the events emitted from the http request, the finish event means "the request has been sent. More specifically, this event is emitted when the last segment of the response headers and body have been handed off to the operating system for transmission over the network." I don't think that tweaking the event listeners in this section like I was trying will help at all, as if the request has truly been finished then I think this section should succeed.

I do think that there needs to be another error event listener added in the waitForResponse method though. And adding one there does indeed fix my issue

Effect-TS / platform

Platform-node http client not handling ECONNRESET socket errors #328