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.

Sign up for GitHub

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

Jump to bottom

Keepalive bugfixes and unify timers strategies between client and server #2760

Merged
merged 12 commits into from
Jun 18, 2024
Merged

Keepalive bugfixes and unify timers strategies between client and server #2760

Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ad598ec
Serverside keepalive error detection and cleanups
davidfiala May 28, 2024
334f0dc
remove comment
davidfiala May 28, 2024
d799a7a
unify server and client keepalive matching comments and discussion on…
davidfiala May 29, 2024
577b4b4
add keepalive server trace back in to match channelz vs non-channelz …
davidfiala May 29, 2024
7883164
return imports back to original order
davidfiala May 29, 2024
19cdc12
another missing trace message for parity
davidfiala May 29, 2024
bed5e85
resolve hoisting
davidfiala May 29, 2024
d325b5f
hoist in second location
davidfiala May 29, 2024
a77d94f
Based on grpc/grpc-node#2139 I wrapped http2session.ping in a try-cat…
davidfiala May 29, 2024
c2da436
remove keepaliveDisabled from server.ts. rename keepaliveTimer.
davidfiala May 29, 2024
3c5ab22
per discussion, avoid tracking keepalive disabled state and instead d…
davidfiala Jun 6, 2024
98cd87f
ensure that client keepalive timers are always cleared when they trig…
davidfiala Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 132 additions & 71 deletions packages/grpc-js/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1376,8 +1376,7 @@ export class Server {

let connectionAgeTimer: NodeJS.Timeout | null = null;
let connectionAgeGraceTimer: NodeJS.Timeout | null = null;
let keeapliveTimeTimer: NodeJS.Timeout | null = null;
let keepaliveTimeoutTimer: NodeJS.Timeout | null = null;
let keepaliveInterval: NodeJS.Timeout | null = null;
let sessionClosedByServer = false;

const idleTimeoutObj = this.enableIdleTimeout(session);
Expand Down Expand Up @@ -1421,41 +1420,72 @@ export class Server {
}

if (this.keepaliveTimeMs < KEEPALIVE_MAX_TIME_MS) {
keeapliveTimeTimer = setInterval(() => {
keepaliveTimeoutTimer = setTimeout(() => {
sessionClosedByServer = true;
session.close();
keepaliveInterval = setInterval(() => {
const keepaliveTimeout = setTimeout(() => {
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
sessionClosedByServer = true;
this.trace('Connection dropped by keepalive timeout');
session.close();
}
}, this.keepaliveTimeoutMs);
keepaliveTimeoutTimer.unref?.();
keepaliveTimeout.unref?.();

try {
session.ping(
(err: Error | null, duration: number, payload: Buffer) => {
if (keepaliveTimeoutTimer) {
clearTimeout(keepaliveTimeoutTimer);
if (
!session.ping(
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
(err: Error | null, duration: number, payload: Buffer) => {
clearTimeout(keepaliveTimeout);
if (err) {
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
}
sessionClosedByServer = true;
this.trace(
'Connection dropped due to error with ping frame ' +
err.message +
' return in ' +
duration
);
session.close();
}
}

if (err) {
sessionClosedByServer = true;
this.trace(
'Connection dropped due to error of a ping frame ' +
err.message +
' return in ' +
duration
);
session.close();
}
}
);
)
) {
throw new Error('Server keepalive ping send failed');
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
}
} catch (e) {
clearTimeout(keepaliveTimeoutTimer);
// The ping can't be sent because the session is already closed
// The ping can't be sent because the session is already closed, max outstanding pings reached, etc
clearTimeout(keepaliveTimeout);
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
}
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
this.trace(
'Connection dropped due to error sending ping frame ' +
(e instanceof Error ? e.message : 'unknown error')
);
session.destroy();
}
}, this.keepaliveTimeMs);
keeapliveTimeTimer.unref?.();
keepaliveInterval.unref?.();
}

session.once('goaway', (errorCode, lastStreamID, opaqueData) => {
if (errorCode === http2.constants.NGHTTP2_ENHANCE_YOUR_CALM) {
this.trace('Connection dropped by client due to ENHANCE_YOUR_CALM');
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
} else {
this.trace(
'Connection dropped by client via GOAWAY with error code ' +
errorCode
);
}
sessionClosedByServer = true;
davidfiala marked this conversation as resolved.
Show resolved Hide resolved
session.destroy();
});

session.on('close', () => {
if (!sessionClosedByServer) {
this.trace(
Expand All @@ -1471,11 +1501,9 @@ export class Server {
clearTimeout(connectionAgeGraceTimer);
}

if (keeapliveTimeTimer) {
clearInterval(keeapliveTimeTimer);
if (keepaliveTimeoutTimer) {
clearTimeout(keepaliveTimeoutTimer);
}
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
}

if (idleTimeoutObj !== null) {
Expand Down Expand Up @@ -1521,8 +1549,7 @@ export class Server {

let connectionAgeTimer: NodeJS.Timeout | null = null;
let connectionAgeGraceTimer: NodeJS.Timeout | null = null;
let keeapliveTimeTimer: NodeJS.Timeout | null = null;
let keepaliveTimeoutTimer: NodeJS.Timeout | null = null;
let keepaliveInterval: NodeJS.Timeout | null = null;
let sessionClosedByServer = false;

const idleTimeoutObj = this.enableIdleTimeout(session);
Expand Down Expand Up @@ -1565,49 +1592,85 @@ export class Server {
}

if (this.keepaliveTimeMs < KEEPALIVE_MAX_TIME_MS) {
keeapliveTimeTimer = setInterval(() => {
keepaliveTimeoutTimer = setTimeout(() => {
sessionClosedByServer = true;
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped by keepalive timeout from ' + clientAddress
);

session.close();
keepaliveInterval = setInterval(() => {
const keepaliveTimeout = setTimeout(() => {
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
sessionClosedByServer = true;
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped by keepalive timeout from ' + clientAddress
);
session.close();
}
}, this.keepaliveTimeoutMs);
keepaliveTimeoutTimer.unref?.();
keepaliveTimeout.unref?.();

try {
session.ping(
(err: Error | null, duration: number, payload: Buffer) => {
if (keepaliveTimeoutTimer) {
clearTimeout(keepaliveTimeoutTimer);
}

if (err) {
sessionClosedByServer = true;
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped due to error of a ping frame ' +
err.message +
' return in ' +
duration
);

session.close();
if (
!session.ping(
(err: Error | null, duration: number, payload: Buffer) => {
clearTimeout(keepaliveTimeout);
if (err) {
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
}
sessionClosedByServer = true;
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped due to error with ping frame ' +
err.message +
' return in ' +
duration
);
session.close();
}
}
}
);
)
) {
throw new Error('Server keepalive ping send failed');
}
channelzSessionInfo.keepAlivesSent += 1;
} catch (e) {
clearTimeout(keepaliveTimeoutTimer);
// The ping can't be sent because the session is already closed
// The ping can't be sent because the session is already closed, max outstanding pings reached, etc
clearTimeout(keepaliveTimeout);
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
}
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped due to error sending ping frame ' +
(e instanceof Error ? e.message : 'unknown error')
);
session.destroy();
}
}, this.keepaliveTimeMs);
keeapliveTimeTimer.unref?.();
keepaliveInterval.unref?.();
}

session.once('goaway', (errorCode, lastStreamID, opaqueData) => {
if (errorCode === http2.constants.NGHTTP2_ENHANCE_YOUR_CALM) {
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped by client due GOAWAY of ENHANCE_YOUR_CALM from ' +
clientAddress
);
} else {
this.channelzTrace.addTrace(
'CT_INFO',
'Connection dropped by client via GOAWAY with error code ' +
errorCode +
' from ' +
clientAddress
);
}
sessionClosedByServer = true;
session.destroy();
});

session.on('close', () => {
if (!sessionClosedByServer) {
this.channelzTrace.addTrace(
Expand All @@ -1627,11 +1690,9 @@ export class Server {
clearTimeout(connectionAgeGraceTimer);
}

if (keeapliveTimeTimer) {
clearInterval(keeapliveTimeTimer);
if (keepaliveTimeoutTimer) {
clearTimeout(keepaliveTimeoutTimer);
}
if (keepaliveInterval) {
clearInterval(keepaliveInterval);
keepaliveInterval = null;
}

if (idleTimeoutObj !== null) {
Expand Down