Add optional timeoutMs parameter to makeApiClient #2430
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jannikschaper The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @jannikschaper! |
k8s-ci-robot
added
size/M
|
|
||
| const resultPromise = fetch(request.getUrl(), { | ||
| method: method, | ||
| body: body as any, |
There was a problem hiding this comment.
| body: body as any, | |
| body: body as BodyInit, |
If we do this typecast we should at least cast into the correct type.
There was a problem hiding this comment.
Could do that. Would lose the pure symmetry with the original code then. If you're aware that I deliberately copied the send implementation verbatim, trying to stay close to the original code, but still prefer to have the more correct type cast, I can do that!
| binary: () => resp.buffer(), | ||
| }; | ||
| return new ResponseContext(resp.status, headers, body); | ||
| }); |
There was a problem hiding this comment.
Not 100% sure but do we need a catch here as well or will this be catched by the client code?
There was a problem hiding this comment.
This code, as is (without a catch) should behave identically to the status quo. I copied the send implementation verbatim from ./gen/http/isomorphic-fetch.ts (as noted above); only adding the timeout: options?.timeoutMs, line. Since the original code didn't need a catch, I argue this code must not need it either
| // This HTTP API implementation is a copy of ./gen/http/isomorphic-fetch.ts, | ||
| // extended with the timeout pass-through code. | ||
| httpApi: { | ||
| send(request: RequestContext): Observable<ResponseContext> { |
There was a problem hiding this comment.
I don't love pulling this full implementation into this library. Is there an easier way to inject the timeout?
There was a problem hiding this comment.
I thought that too, it would be much nicer to inject the extra field without having to rip out the entire function. But I believe there's no other way. The auto-generated code passes the configuration object (without any timeout value, not even passed through) directly into fetch. Not even a dependency-injected fetch or anything that might be overwriteable, but a fetch directly imported from node-fetch.
Expand code screenshot
Maybe it's still possible to reach into the code via JavaScript black magic to overwrite the fetch implementation and amend the timeout parameter, but I have a hunch such code will be even uglier than copying the 22 lines of code
There was a problem hiding this comment.
What do you think about changing the code generator itself? They're usually pretty good about merging changes quickly into the generator.
There was a problem hiding this comment.
I wanted to avoid that seeming rabbit hole. But maybe it is a better solution, I will look into it
|
i'm testing out a PR on the generator for passing through the signal property on the middleware has access to the RequestContext which would allow adding a middleware that sets the abortsignal another alternative is to add a |
|
@jannikschaper how do you feel about the middleware injection approach? something like this: /**
* Middleware that adds an abort signal to the request context.
* This can be used to abort requests using an AbortController.
* @param signal AbortSignal to use for the request
* @returns Middleware that sets the signal in the request context
*/
function abortSignalMiddleware(signal: AbortSignal): Middleware {
return {
pre: async (c: RequestContext) => {
c.setSignal(signal)
return c
},
post: async (c: ResponseContext) => {
return c
},
}
} |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
It works! const makeApiClient = <T extends ApiType>(
kubeConfig: KubeConfig,
ApiClientType: ApiConstructor<T>,
requestTimeoutInSeconds: number,
): T => {
// Copied from KubeConfig.makeApiClient and adapted to add timeout
// Suggested by @kubernetes/client-node maintainer (https://github.com/kubernetes-client/javascript/pull/2430#issuecomment-2918109624)
const cluster = kubeConfig.getCurrentCluster();
if (!cluster) {
throw new Error('No active cluster!');
}
return new ApiClientType(createConfiguration({
baseServer: new ServerConfiguration(cluster.server, {}),
authMethods: {
default: kubeConfig,
},
middleware: [{
pre: (context) => {
const controller = new AbortController();
setTimeout(() => controller.abort(), requestTimeoutInSeconds * 1000);
context.setSignal(controller.signal);
return new Observable(Promise.resolve(context));
},
post: (context) => new Observable(Promise.resolve(context)),
}],
}));
};Thank you so much @davidgamero for going out of your way and getting PRs merged both here and even in OpenAPI to fix my use case. Using a signal is better than a hardcoded timeout value, because you can implement a timeout and more on top of it, good idea. Thanks! |
|
@davidgamero I have a follow-up question to the timeout approach you suggested, if you don't mind How to clean up dangling timeout instances when requests complete normally? For example, with a 120s timeout and on average 10 successful Kubernetes API requests per second, there would be over a thousand timeout promises still hanging around in the NodeJS event loop. Each of them also eventually calling A naive implementation might look like this: function timeoutMiddleware(milliseconds: number): Middleware {
let timeoutId: NodeJS.Timeout;
return {
pre: async (c: RequestContext) => {
const abortController = new AbortController();
timeoutId = setTimeout(() => abortController.abort(), milliseconds);
c.setSignal(abortController.signal);
return c;
},
post: async (c: ResponseContext) => {
clearTimeout(timeoutId);
return c;
},
};
}But this is wrong, because parallel requests would overwrite each other's Do you have an idea how to solve the problem of dangling timeouts with this library? Is there simply no way, and the best solution is to just deal with dangling timeouts aborting each not-anymore-existent request long after it finished? Thank you for your help! |
|
@jannikschaper We have found a solution for that: promiseMiddleware: [{
pre: async (context) => {
context.setSignal(AbortSignal.timeout(requestTimeoutInSeconds * 1000));
return context;
},
post: async (context) => context,
}],This way, we don't have to cleanup the timeout ourself because NodeJs internally calls |
7 participants
Fixes #1613
I followed @brendandburns comments from the linked issue: