Skip to content

Eliminate IdV async polling delay in test#6554

Merged
aduth merged 4 commits intomainfrom
aduth-try-no-poll-delay
Jul 8, 2022
Merged

Eliminate IdV async polling delay in test#6554
aduth merged 4 commits intomainfrom
aduth-try-no-poll-delay

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 7, 2022

Why: To improve overall test runtime.

In all cases, this polling will wait for a response to be received and handled before issuing a new request, so there should be no risk of overlapping requests overwhelming the local server.

aduth added 3 commits July 7, 2022 08:10
**Why**: It could improve overall test runtime.

In all cases, this polling will wait for a response to be received and handled before issuing a new request, so there should be no risk of overlapping requests overwhelming the local server.
changelog: Internal, Automated Testing, Improve automated testing performance
The `|| undefined` logic will be tripped for 0. The intention was to try to avoid scheduling a poll if the result of "Number" parsing was "NaN", since that would be treated the same as "0" in setTimeout, thus potentially causing a DDOS. Instead, check for finiteness of the value when scheduling.
function handleResponse() {
if (response.isPending) {
if (statusPollInterval !== undefined) {
if (Number.isFinite(statusPollInterval)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

More context for these changes in the extended commit description of 484665c:

The || undefined logic will be tripped for 0. The intention was to try to avoid scheduling a poll if the result of "Number" parsing was "NaN", since that would be treated the same as "0" in setTimeout, thus potentially causing a DDOS. Instead, check for finiteness of the value when scheduling.

Number.isFinite(1)
// true
Number.isFinite(0)
// true
Number.isFinite(NaN)
// false
Number.isFinite(undefined)
// false

@aduth
Copy link
Contributor Author

aduth commented Jul 7, 2022

Looking at median "Specs" job runtime in GitLab over two runs each before and after (Before1, Before2, After1, After2), looks like there's some small improvement here.

Before: 15:20
After: 14:51

We may also need to regenerate the Knapsack reports again to more accurately measure this.

@aduth aduth changed the title Try eliminating IdV async polling delay in test Eliminate IdV async polling delay in test Jul 7, 2022
@aduth aduth marked this pull request as ready for review July 7, 2022 14:31
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth aduth merged commit 497a86e into main Jul 8, 2022
@aduth aduth deleted the aduth-try-no-poll-delay branch July 8, 2022 12:02
@jmdembe jmdembe mentioned this pull request Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants