-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Add command timeout #2981
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
Changes from 4 commits
4b89ef9
0b98f24
e8eb95b
19e4935
dd72fea
3c9b0fc
0d16695
f2a3076
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ import { BasicAuth, CredentialsError, CredentialsProvider, StreamingCredentialsP | |
import RedisCommandsQueue, { CommandOptions } from './commands-queue'; | ||
import { EventEmitter } from 'node:events'; | ||
import { attachConfig, functionArgumentsPrefix, getTransformReply, scriptArgumentsPrefix } from '../commander'; | ||
import { ClientClosedError, ClientOfflineError, DisconnectsClientError, WatchError } from '../errors'; | ||
import { ClientClosedError, ClientOfflineError, CommandTimeoutError, DisconnectsClientError, WatchError } from '../errors'; | ||
import { URL } from 'node:url'; | ||
import { TcpSocketConnectOpts } from 'node:net'; | ||
import { PUBSUB_TYPE, PubSubType, PubSubListener, PubSubTypeListeners, ChannelListeners } from './pub-sub'; | ||
|
@@ -144,6 +144,10 @@ export interface RedisClientOptions< | |
* Tag to append to library name that is sent to the Redis server | ||
*/ | ||
clientInfoTag?: string; | ||
/** | ||
* Provides a timeout in milliseconds. | ||
*/ | ||
commandTimeout?: number; | ||
} | ||
|
||
type WithCommands< | ||
|
@@ -889,9 +893,41 @@ export default class RedisClient< | |
return Promise.reject(new ClientOfflineError()); | ||
} | ||
|
||
let controller: AbortController; | ||
if (this._self.#options?.commandTimeout) { | ||
controller = new AbortController() | ||
let abortSignal = controller.signal; | ||
if (options?.abortSignal) { | ||
abortSignal = AbortSignal.any([ | ||
abortSignal, | ||
options.abortSignal | ||
]); | ||
} | ||
options = { | ||
...options, | ||
abortSignal | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can simplify this by using the AbortSignal.timeout static method:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried this, also together with the change in the test that you proposed, but somehow for me the test keeps failing with this approach. I am not quite understanding, why. |
||
const promise = this._self.#queue.addCommand<T>(args, options); | ||
|
||
this._self.#scheduleWrite(); | ||
return promise; | ||
if (!this._self.#options?.commandTimeout) { | ||
return promise; | ||
} | ||
|
||
return new Promise<T>((resolve, reject) => { | ||
const timeoutId = setTimeout(() => { | ||
controller.abort(); | ||
reject(new CommandTimeoutError()); | ||
}, this._self.#options?.commandTimeout) | ||
promise.then(result => { | ||
clearInterval(timeoutId); | ||
resolve(result) | ||
}).catch(error => { | ||
clearInterval(timeoutId); | ||
reject(error) | ||
}); | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, this change becomes obsolete |
||
} | ||
|
||
async SELECT(db: number): Promise<void> { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,12 @@ export class SocketTimeoutError extends Error { | |
} | ||
} | ||
|
||
export class CommandTimeoutError extends Error { | ||
constructor() { | ||
super('Command timeout'); | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to introduce another error, we can use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I changed it to use AbortError, but couldn't there be situation where it makes sense to distinguish between a timeout and having the command manually aborted? Wouldn't it then make more sense to have a different error? |
||
export class ClientClosedError extends Error { | ||
constructor() { | ||
super('The client is closed'); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test would need to change like this: