Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions packages/wdio-types/src/Options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ export interface WebDriver extends Connection {
* @default 3
*/
connectionRetryCount?: number
/**
* Base delay in ms for exponential backoff on 429 responses during session creation.
* If set, retries use exponential backoff instead of immediate retry.
*/
exp429RetryBaseDelay?: number

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the option and leaving only the constant value. In my opinion, there is no reason for a new option.

/**
* Specify custom headers to pass into every request.
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/webdriver/src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ export const DEFAULTS: Options.Definition<Required<RemoteConfig>> = {
type: 'number',
default: 3
},
exp429RetryBaseDelay: {
type: 'number'
},
/**
* Override default agent
*/
Expand Down
13 changes: 12 additions & 1 deletion packages/webdriver/src/request/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export default abstract class WebDriverRequest extends EventEmitter {
fullRequestOptions = options.transformRequest(fullRequestOptions)
}

fullRequestOptions.exp429RetryBaseDelay = options.exp429RetryBaseDelay
this.emit('request', fullRequestOptions)
return this._request(fullRequestOptions, options.transformResponse, options.customWdRequestAgent, options.connectionRetryCount, 0)
}
Expand Down Expand Up @@ -178,7 +179,7 @@ export default abstract class WebDriverRequest extends EventEmitter {
log.info('DATA', transformCommandLogResult(fullRequestOptions.json))
}

const { url, retry: _, ...requestLibOptions } = fullRequestOptions
const { url, retry: _, exp429RetryBaseDelay: __, ...requestLibOptions } = fullRequestOptions
const startTime = this._libPerformanceNow()
let response = customWdRequestAgent
? await customWdRequestAgent.request(url!, requestLibOptions).catch((err: RequestLibError) => err)
Expand Down Expand Up @@ -207,6 +208,16 @@ export default abstract class WebDriverRequest extends EventEmitter {
this.emit('performance', { request: fullRequestOptions, durationMillisecond, success: false, error, retryCount })
log.warn(msg)
log.info(`Retrying ${retryCount}/${totalRetryCount}`)

const exp429RetryBaseDelay = fullRequestOptions.exp429RetryBaseDelay

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest changing the name of this variable it's too complicated

if (exp429RetryBaseDelay && !(response instanceof Error) && response.statusCode === 429) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we here retraying absolutely all requests, not just for creating a session?

const delay = Math.round(exp429RetryBaseDelay * 2 ** (retryCount - 1) + Math.random() * 100)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is the jitter so low? Only 100ms

log.warn(`Session creation failed with 429, retrying in ${delay}ms`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we need a warning here, not a debug log?

return new Promise(resolve => setTimeout(resolve, delay)).then(
() => this._request(fullRequestOptions, transformResponse, customWdRequestAgent, totalRetryCount, retryCount)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just delete this call and use the call of this._request below?

)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if tomorrow we let the user manage connectionRetryCount option? And he set value 10. How much we should wait? I'm saying that we need some kind of maximum limit.


return this._request(fullRequestOptions, transformResponse, customWdRequestAgent, totalRetryCount, retryCount)
}

Expand Down
1 change: 1 addition & 0 deletions packages/webdriver/src/request/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,5 @@ export interface RequestLibOptions {
username?: string
password?: string
body?: unknown
exp429RetryBaseDelay?: number
}
2 changes: 1 addition & 1 deletion packages/webdriver/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export async function startWebDriverSession (params: RemoteConfig): Promise<{ se

let response: SessionInitializationResponse
try {
response = await sessionRequest.makeRequest(params) as SessionInitializationResponse
response = await sessionRequest.makeRequest({ ...params, exp429RetryBaseDelay: params.exp429RetryBaseDelay ?? 5000 }) as SessionInitializationResponse
} catch (err) {
log.error(err)
const message = getSessionError(err as Error, params)
Expand Down
Loading