Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[internal] The session has been destroyed #683

Closed
akosyakov opened this issue Jun 21, 2023 · 12 comments · Fixed by #783
Closed

[internal] The session has been destroyed #683

akosyakov opened this issue Jun 21, 2023 · 12 comments · Fixed by #783
Labels
bug Something isn't working

Comments

@akosyakov
Copy link
Contributor

version: 0.10.0

ConnectError: [internal] The session has been destroyed
	at i.from (/out/extension.js:2:1820120)
	at t.connectErrorFromNodeReason (/out/extension.js:2:1804959)
	at Promise.reject (/out/extension.js:2:1809452)
	at /out/extension.js:2:1811572

Unfortunately stack trace is minified on our side not sure which path lead to that, it seem to be an unary call. It seems somewhere stream is called when it is already destroyed.

@akosyakov akosyakov added the bug Something isn't working label Jun 21, 2023
@timostamm
Copy link
Member

The issue might be related to #681, which was just fixed in v0.10.1, but it's impossible to tell without more information.

Can you enable source maps in your build so that it's possible to restore stack traces?

@akosyakov
Copy link
Contributor Author

I tried to recover and got following stack trace:

i.from at ../node_modules/@bufbuild/connect/dist/cjs/connect-error.js:73:19
from at ../node_modules/@bufbuild/connect-node/dist/cjs/node-error.js:54:38
reason at ../node_modules/@bufbuild/connect-node/dist/cjs/node-universal-client.js:305:110
reject at ../node_modules/@bufbuild/connect-node/dist/cjs/node-universal-client.js:204:17

@akosyakov
Copy link
Contributor Author

so it is coming from here: https://github.com/bufbuild/connect-es/blob/76b17edbbc4c0d4c98d1de8eb754192835458054/packages/connect-node/src/node-universal-client.ts#L340

the rest of stack trace is wrapping an error in connect error

@timostamm
Copy link
Member

That's good info. Since the stack does not include gotoReady, it means the error is raised here:

https://github.com/bufbuild/connect-es/blob/b6187e9b2007bf178c0a6dea79cab3f30ff09c1a/packages/connect-node/src/http2-session-manager.ts#L201

This means the connection owned by the ready state has been destroyed without the state noticing. Looking at the source, I don't see a smoking gun, we're maintaining event listeners on "close" and "error" all the time.

It's also possible that the connection is already dead if we enter the "ready" state. We noticed some unexpected behavior with the http2 module while the connection is established, see https://github.com/bufbuild/connect-es/blob/b6187e9b2007bf178c0a6dea79cab3f30ff09c1a/packages/connect-node/src/http2-session-manager.ts#L415-L417
#687 adds an assertion so that we can identify whether this is the case.

@akosyakov
Copy link
Contributor Author

fyi we upgrade to 0.10.1 but we still can see it happens for some users, so it does not seem to go away with #681

@akosyakov
Copy link
Contributor Author

@timostamm We also still see New streams cannot be created after receiving a GOAWAY as internal errors. Are they expected to be thrown happen after recent fixes? as internal?

@timostamm
Copy link
Member

@akosyakov, with #681, a new call will open a new connection if the current connection has received a GOAWAY. The error you see is unexpected. Will update #680 with the new information.

@timostamm
Copy link
Member

I'm able to reproduce:

$ node -v
v18.17.0
$ node repro-683.mjs
http2.connect
conn on connect
conn.request
req on finish
req on response 200
conn on goaway, code: 2
req on error Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 2
req on close
connection after request closed: closed: false destroyed: true
conn on error Error [ERR_HTTP2_SESSION_ERROR]: Session closed with error code 2
conn on close
repro-683.mjs
import * as http2 from "http2";

// start a server that sends a GOAWAY with INTERNAL ERROR to every session
const PORT = 8088;
const server = http2.createServer()
    .on("session", (sess) => {
        setTimeout(() => {
            sess.goaway(http2.constants.NGHTTP2_INTERNAL_ERROR)
        }, 100);
    })
    .on("request", (req, res) => {
        res.writeHead(200);
        setTimeout(() => res.end(), 200);
    });
await new Promise(resolve => {
    server.listen(PORT, () => resolve());
});


// open a connection
const conn = await new Promise((resolve, reject) => {
    console.log("http2.connect");
    const conn = http2.connect(`http://localhost:${PORT}`);
    conn.on("error", reject);
    conn.on("connect", () => {
        console.log("conn on connect");
        resolve(conn);
    });
    conn.on("goaway", errorCode => {
        console.log("conn on goaway, code:", errorCode);
    });
    conn.on("close", () => {
        console.log("conn on close")
    });
    conn.on("error", e => {
        console.log("conn on error", String(e));
    });
});

// make a request
await new Promise((resolve) => {
    console.log("conn.request");
    const stream = conn.request({
        ":method": "POST",
        ":path": "/",
    });
    stream.on("response", h => {
        console.log("req on response", h[":status"]);
    });
    stream.on("error", e => {
        console.log("req on error", String(e));
    });
    stream.on("finish", () => {
        console.log("req on finish");
    });
    stream.on("close", () => {
        console.log("req on close");
        resolve();
    });
    stream.on("close", resolve);
    stream.end();
});

console.log("connection after request closed: closed:", conn.closed, "destroyed:", conn.destroyed);

await new Promise(resolve => setTimeout(resolve, 500))
process.exit(0);

When the http2.ClientHttp2Session receives the GOAWAY frame with INTERNAL_ERROR (conn on goaway), open streams are destroyed (req on error, req on close).

Since the request received the "error" and "close" events, it is complete, and user-code will continue. For example with a second request. But at this point, the session is destroyed (destroyed: true), and a second request will raise the error in the issue title. The session manager does not know at this point, because it has not received an "error" event yet - it will follow some time later.

@timostamm
Copy link
Member

timostamm commented Jul 25, 2023

v0.12.0 was just published with #712.

We can reproduce the fix with the following script that uses the session manager:

repro.mjs
import * as http2 from "http2";
import { createNodeHttpClient, Http2SessionManager } from "@bufbuild/connect-node";
import assert from "assert";

// start a server that sends a GOAWAY with INTERNAL ERROR to every session
const PORT = 8088;
const server = http2.createServer()
    .on("session", (sess) => {
        setTimeout(() => {
            sess.goaway(http2.constants.NGHTTP2_INTERNAL_ERROR)
        }, 100);
    })
    .on("request", (req, res) => {
        res.writeHead(200);
        setTimeout(() => res.end(), 200);
    });
await new Promise(resolve => {
    server.listen(PORT, () => resolve());
});


// create an h2 client
const sm = new Http2SessionManager(`http://localhost:${PORT}`);
const client = createNodeHttpClient({
    httpVersion: "2",
    sessionProvider: () => sm
});


// make requests
const res1 = await client({
    url: `http://localhost:${PORT}`,
    method: "POST",
    header: new Headers(),
});
try {
    for await (const chunk of res1.body) {
        // consume response body to see error
    }
} catch (e) {
    assert(String(e) === "ConnectError: [internal] Session closed with error code 2");
}

console.log("res1 done")

const res2 = await client({
    url: `http://localhost:${PORT}`,
    method: "POST",
    header: new Headers(),
});

console.log("res2 done")
$ npm i @bufbuild/connect-node@0.11.0
$ node repro.mjs
res1 done
file:///Users/ts/Downloads/xrepro/node_modules/@bufbuild/connect/dist/esm/connect-error.js:70
            return new ConnectError(reason.message, code);
                   ^

ConnectError: [internal] The session has been destroyed
...
$ npm i @bufbuild/connect-node@0.12.0
$ node repro.mjs
res1 done
res2 done
^C

I'm going to close this issue as resolved. But in case you do still experience issues, let us know. Without a reproducible case, issues like this are impossible to fix while maintaining code quality though.

@jeanp413
Copy link

jeanp413 commented Aug 8, 2023

@timostamm we released a new version of our extension with the connect library updated to 0.0.12 and the errors are back 😢 , with the original changes from #712 we no longer had these error reports.
Could you reopen this 🙏

@timostamm
Copy link
Member

Thanks for the confirmation, @akosyakov and @jeanp413. I was afraid that might happen.

We discussed the issue and we are okay with applying a broad workaround. It will take use a couple of days. In the meantime, the recommended course is to pin to v0.11.0 if you are experiencing this issue.

@timostamm
Copy link
Member

timostamm commented Aug 29, 2023

The new fix just shipped in v0.13.2. Let us know if this fixes the problem, @akosyakov and @jeanp413.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants