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.

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

Fix incompatibility with New Relic #351

Merged
merged 4 commits into from
May 28, 2021
Merged

Conversation

lukeupup
Copy link
Contributor

@lukeupup lukeupup commented May 25, 2021

Fix #345

} catch (error: unknown) {
reject(error);
}
}, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't change 1 to 0, though 0 looks also work

Copy link
Owner

Choose a reason for hiding this comment

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

I don't remember why it's 1 and not 0.

@sholladay Do you?

Copy link
Owner

Choose a reason for hiding this comment

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

If 0 is fine, we can just remove the argument altogether.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if Chrome replaces 0 with 1 or no. In that case it wouldn't matter.

source/core/Ky.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

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

I don't think you need to wrap the for loop in the new promise, just the fetch.

@sindresorhus
Copy link
Owner

Please report this to New Relic and then update the code comment with a bug ID.

}
}
return new Promise<Response>((resolve, reject) => {
// Fix #345: We must keep setTimeout() here instead of using delay(), because new relic cannot handle parallel promise correctly.
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure what you mean by "parallel promise". Would be great if you could be more specific.

@sindresorhus
Copy link
Owner

Can you check if queueMicrotask() works?

@sholladay
Copy link
Collaborator

@lukeupup I pushed some changes to make the code a bit cleaner. I'm hoping it will still work but have no way to test it. Can you please check?

@lukeupup
Copy link
Contributor Author

@lukeupup I pushed some changes to make the code a bit cleaner. I'm hoping it will still work but have no way to test it. Can you please check?

@sholladay Amazing, it works indeed on our environment! Although I don't really understand how new relic treat delay and Promise.resolve differently... Not sure if @smblee can see this thread, but it would be better to invite him to have some test as well. If it also works, I think we can solve the problem as you've submitted. Thanks a lot!

@szmarczak
Copy link
Collaborator

@lukeupup In case you missed this: #351 (comment)

@smblee
Copy link

smblee commented May 27, 2021

Just catching up on this thread - we switched to axios for the time being but i'll give this a shot!

@sholladay
Copy link
Collaborator

My theory is that the bug has something to do with the fact that delay() first schedules a callback in the task queue and then after that fires it schedules a callback in the job (microtask) queue. Doing one or the other is normal but combined it's pretty unusual so I figure that may be a situation that New Relic's code isn't handling well. It happens because delay() is using both setTimeout() and a promise.

I don't see a clean way to use queueMicrotask() here as the code is async. We'd have to wrap queueMicrotask() in a promise, which would effectively queue two microtasks. Seems pointless. That said, I updated the code to use await Promise.resolve(), which is very similar to queueMicrotask().

BTW, I also updated AVA to reduce the amount of noisy console logs during testing.

I think this PR is ready to merge, assuming it works for @smblee.

@lukeupup
Copy link
Contributor Author

@sholladay This looks convincing. So let's look forward to the verification result from @smblee . Is there anything I should do before we can merge the code?

@smblee
Copy link

smblee commented May 28, 2021

I can confirm that this definitely addresses it! Thanks!

@sindresorhus sindresorhus changed the title Fix #345: Replace delay with setTimeout to fix the problem that NewRelic fails to log requests Fix incompatibility with New Relic May 28, 2021
@sindresorhus sindresorhus merged commit 5f3c315 into sindresorhus:main May 28, 2021
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.

1ms delay before fetch causing issues with New Relic SPA logger
5 participants