Skip to content

Commit 2d1cade

Browse files
trivikrtrivikr
committed
chore: remove sessionList
1 parent 241403b commit 2d1cade

File tree

2 files changed

+54
-66
lines changed

2 files changed

+54
-66
lines changed

‎packages/node-http-handler/src/node-http2-handler.spec.ts

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -217,19 +217,16 @@ describe(NodeHttp2Handler.name, () => {
217217
it("destroys session and clears sessionCache", async () => {
218218
await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {});
219219

220+
const authority = `${protocol}//${hostname}:${port}`;
220221
// @ts-ignore: access private property
221-
const session: ClientHttp2Session = nodeH2Handler.sessionList[0];
222+
const session: ClientHttp2Session = nodeH2Handler.sessionCache.get(authority)[0];
222223

223224
// @ts-ignore: access private property
224225
expect(nodeH2Handler.sessionCache.size).toBe(1);
225-
// @ts-ignore: access private property
226-
expect(nodeH2Handler.sessionList.length).toBe(1);
227226
expect(session.destroyed).toBe(false);
228227
nodeH2Handler.destroy();
229228
// @ts-ignore: access private property
230229
expect(nodeH2Handler.sessionCache.size).toBe(0);
231-
// @ts-ignore: access private property
232-
expect(nodeH2Handler.sessionList.length).toBe(0);
233230
expect(session.destroyed).toBe(true);
234231
});
235232
});
@@ -254,8 +251,9 @@ describe(NodeHttp2Handler.name, () => {
254251
// Create a session by sending a request.
255252
await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {});
256253

254+
const authority = `${protocol}//${hostname}:${port}`;
257255
// @ts-ignore: access private property
258-
const session: ClientHttp2Session = nodeH2Handler.sessionList[0];
256+
const session: ClientHttp2Session = nodeH2Handler.sessionCache.get(authority)[0];
259257
const requestSpy = jest.spyOn(session, "request");
260258

261259
await expect(
@@ -335,31 +333,39 @@ describe(NodeHttp2Handler.name, () => {
335333
describe("sessionTimeout", () => {
336334
const sessionTimeout = 200;
337335

338-
describe("destroys session on sessionTimeout", () => {
336+
describe("destroys sessions on sessionTimeout", () => {
339337
it("disableConcurrentStreams: false (default)", async (done) => {
340338
nodeH2Handler = new NodeHttp2Handler({ sessionTimeout });
341339
await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {});
342340

343341
const authority = `${protocol}//${hostname}:${port}`;
344342
// @ts-ignore: access private property
345-
const session: ClientHttp2Session = nodeH2Handler.sessionList[0];
343+
const session: ClientHttp2Session = nodeH2Handler.sessionCache.get(authority)[0];
346344
expect(session.destroyed).toBe(false);
347345
// @ts-ignore: access private property
348-
expect(nodeH2Handler.sessionCache.get(authority)).toBeDefined();
346+
expect(nodeH2Handler.sessionCache.get(authority).length).toStrictEqual(1);
349347
setTimeout(() => {
350348
expect(session.destroyed).toBe(true);
351349
// @ts-ignore: access private property
352-
expect(nodeH2Handler.sessionCache.get(authority)).not.toBeDefined();
350+
expect(nodeH2Handler.sessionCache.get(authority).length).toStrictEqual(0);
353351
done();
354352
}, sessionTimeout + 100);
355353
});
356354

357355
it("disableConcurrentStreams: true", async (done) => {
356+
let session;
357+
const authority = `${protocol}//${hostname}:${port}`;
358+
358359
nodeH2Handler = new NodeHttp2Handler({ sessionTimeout, disableConcurrentStreams: true });
360+
361+
mockH2Server.removeAllListeners("request");
362+
mockH2Server.on("request", (request: any, response: any) => {
363+
// @ts-ignore: access private property
364+
session = nodeH2Handler.sessionCache.get(authority)[0];
365+
createResponseFunction(mockResponse)(request, response);
366+
});
359367
await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {});
360368

361-
// @ts-ignore: access private property
362-
const session: ClientHttp2Session = nodeH2Handler.sessionList[0];
363369
expect(session.destroyed).toBe(false);
364370
setTimeout(() => {
365371
expect(session.destroyed).toBe(true);
@@ -428,19 +434,14 @@ describe(NodeHttp2Handler.name, () => {
428434
});
429435

430436
describe("destroy", () => {
431-
it("destroys session and empties sessionList", async () => {
437+
it("destroys session and empties sessionCache", async () => {
432438
await nodeH2Handler.handle(new HttpRequest(getMockReqOptions()), {});
433439

434440
// @ts-ignore: access private property
435-
const session: ClientHttp2Session = nodeH2Handler.sessionList[0];
436-
437-
// @ts-ignore: access private property
438-
expect(nodeH2Handler.sessionList.length).toBe(1);
439-
expect(session.destroyed).toBe(false);
441+
expect(nodeH2Handler.sessionCache.size).toBe(1);
440442
nodeH2Handler.destroy();
441443
// @ts-ignore: access private property
442-
expect(nodeH2Handler.sessionList.length).toBe(0);
443-
expect(session.destroyed).toBe(true);
444+
expect(nodeH2Handler.sessionCache.size).toBe(0);
444445
});
445446
});
446447
});

‎packages/node-http-handler/src/node-http2-handler.ts

Lines changed: 33 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -38,19 +38,19 @@ export class NodeHttp2Handler implements HttpHandler {
3838
private readonly disableConcurrentStreams?: boolean;
3939

4040
public readonly metadata = { handlerProtocol: "h2" };
41-
private sessionList: ClientHttp2Session[];
42-
private sessionCache: Map<string, ClientHttp2Session>;
41+
private sessionCache: Map<string, ClientHttp2Session[]>;
4342

4443
constructor({ requestTimeout, sessionTimeout, disableConcurrentStreams }: NodeHttp2HandlerOptions = {}) {
4544
this.requestTimeout = requestTimeout;
4645
this.sessionTimeout = sessionTimeout;
4746
this.disableConcurrentStreams = disableConcurrentStreams;
48-
this.sessionList = [];
49-
this.sessionCache = new Map<string, ClientHttp2Session>();
47+
this.sessionCache = new Map<string, ClientHttp2Session[]>();
5048
}
5149

5250
destroy(): void {
53-
this.sessionList.forEach((session) => this.destroySession(session));
51+
for (const sessions of this.sessionCache.values()) {
52+
sessions.forEach((session) => this.destroySession(session));
53+
}
5454
this.sessionCache.clear();
5555
}
5656

@@ -71,7 +71,7 @@ export class NodeHttp2Handler implements HttpHandler {
7171

7272
const { hostname, method, port, protocol, path, query } = request;
7373
const authority = `${protocol}//${hostname}${port ? `:${port}` : ""}`;
74-
const session = this.disableConcurrentStreams ? this.getSession(authority) : this.getSessionFromCache(authority);
74+
const session = this.getSession(authority, this.disableConcurrentStreams || false);
7575

7676
const reject = (err: Error) => {
7777
if (this.disableConcurrentStreams) {
@@ -101,6 +101,7 @@ export class NodeHttp2Handler implements HttpHandler {
101101
// Gracefully closes the Http2Session, allowing any existing streams to complete
102102
// on their own and preventing new Http2Stream instances from being created.
103103
session.close();
104+
this.deleteSessionFromCache(authority, session);
104105
}
105106
});
106107

@@ -133,66 +134,49 @@ export class NodeHttp2Handler implements HttpHandler {
133134
// http2stream.rstCode property. If the code is any value other than NGHTTP2_NO_ERROR (0),
134135
// an 'error' event will have also been emitted.
135136
req.on("close", () => {
137+
if (this.disableConcurrentStreams) {
138+
session.destroy();
139+
}
136140
if (!fulfilled) {
137141
reject(new Error("Unexpected error: http2 request did not get a response"));
138142
}
139143
});
144+
140145
writeRequestBody(req, request);
141146
});
142147
}
143148

144149
/**
145-
* Returns a new session for the given URL.
150+
* Returns a session for the given URL.
151+
*
146152
* @param authority The URL to create a session for.
147-
* @returns A new session for the given URL.
153+
* @param disableConcurrentStreams If true, a new session will be created for each request.
154+
* @returns A session for the given URL.
148155
*/
149-
private getSession(authority: string): ClientHttp2Session {
156+
private getSession(authority: string, disableConcurrentStreams: boolean): ClientHttp2Session {
157+
const sessionCache = this.sessionCache;
158+
const existingSessions = sessionCache.get(authority) || [];
159+
160+
// If concurrent streams are not disabled, we can use the existing session.
161+
if (existingSessions.length > 0 && !disableConcurrentStreams) return existingSessions[0];
162+
150163
const newSession = connect(authority);
151164
const destroySessionCb = () => {
152165
this.destroySession(newSession);
166+
this.deleteSessionFromCache(authority, newSession);
153167
};
154168
newSession.on("goaway", destroySessionCb);
155169
newSession.on("error", destroySessionCb);
156170
newSession.on("frameError", destroySessionCb);
157171

158172
const sessionTimeout = this.sessionTimeout;
159173
if (sessionTimeout) {
160-
newSession.setTimeout(sessionTimeout, () => {
161-
this.destroySession(newSession);
162-
});
174+
newSession.setTimeout(sessionTimeout, destroySessionCb);
163175
}
164176

165-
this.sessionList.push(newSession);
166-
return newSession;
167-
}
168-
169-
/**
170-
* Returns a session for the given URL. If the session is not cached, it will be created.
171-
* @param authority The URL to create a session for.
172-
* @returns A session for the given URL.
173-
*/
174-
private getSessionFromCache(authority: string): ClientHttp2Session {
175-
const connectionPool = this.sessionCache;
176-
const existingSession = connectionPool.get(authority);
177-
if (existingSession) return existingSession;
178-
179-
const newSession = this.getSession(authority);
180-
connectionPool.set(authority, newSession);
181-
const destroySessionCb = () => {
182-
this.deleteSessionFromCache(authority, newSession);
183-
};
184-
newSession.on("goaway", destroySessionCb);
185-
newSession.on("error", destroySessionCb);
186-
newSession.on("frameError", destroySessionCb);
177+
existingSessions.push(newSession);
178+
sessionCache.set(authority, existingSessions);
187179

188-
const sessionTimeout = this.sessionTimeout;
189-
if (sessionTimeout) {
190-
newSession.setTimeout(sessionTimeout, () => {
191-
if (connectionPool.get(authority) === newSession) {
192-
connectionPool.delete(authority);
193-
}
194-
});
195-
}
196180
return newSession;
197181
}
198182

@@ -204,7 +188,6 @@ export class NodeHttp2Handler implements HttpHandler {
204188
if (!session.destroyed) {
205189
session.destroy();
206190
}
207-
this.sessionList = this.sessionList.filter((s) => s !== session);
208191
}
209192

210193
/**
@@ -213,10 +196,14 @@ export class NodeHttp2Handler implements HttpHandler {
213196
* @param session The session to delete.
214197
*/
215198
private deleteSessionFromCache(authority: string, session: ClientHttp2Session): void {
216-
if (this.sessionCache.get(authority) !== session) {
217-
// If the session is not in the pool, it has already been deleted.
199+
const existingSessions = this.sessionCache.get(authority) || [];
200+
if (!existingSessions.includes(session)) {
201+
// If the session is not in the cache, it has already been deleted.
218202
return;
219203
}
220-
this.sessionCache.delete(authority);
204+
this.sessionCache.set(
205+
authority,
206+
existingSessions.filter((s) => s !== session)
207+
);
221208
}
222209
}

0 commit comments

Comments
 (0)