-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
TokenFileWebIdentityCredentials uses sync I/O to load credential #3802
Comments
@gabegorelick if we use readfile then the event loop with not block, and the credentials update will take (possibly) more time. Out of curiosity what is the impact of this issue right now for you? Feel free to open a PR to fix if you want, but we dont see this impact as large currently. The file is only being loaded once. The difference seems minimal. |
I haven't checked to see whether the results of the read are cached. If you're correct and
None that I can currently measure. But it's generally considered bad practice to do synchronous I/O in Node without making it explicit. I'm sure we could construct a synthetic benchmark that shows network requests, for example, being ignored for the length of the https://nodejs.org/en/docs/guides/blocking-vs-non-blocking/#concurrency-and-throughput:
The numbers are of course larger for network I/O, but disk I/O still blocks for some length of time. You typically want to avoid that if possible. |
Hi @gabegorelick changing this to feature-request issue since it is not a bug. Feel free to open a PR to fix this. |
|
Greetings! We’re closing this issue because it has been open a long time and hasn’t been updated in a while and may not be getting the attention it deserves. We encourage you to check if this is still an issue in the latest release and if you find that this is still a problem, please feel free to comment or open a new issue. |
Describe the bug
TokenFileWebIdentityCredentials.load
, which is called byrefresh()
, callsfs.readFileSync
. This presumably blocks the event loop.aws-sdk-js/lib/credentials/token_file_web_identity_credentials.js
Line 163 in 33a46b8
Is the issue in the browser/Node.js?
Node.js
If on Node.js, are you running this on AWS Lambda?
No
Details of the browser/Node.js version
v12.22.1
SDK version number
2.929.0
To Reproduce (observed behavior)
I haven't been able to get this to block for any significant amount of time, perhaps because modern disks are so fast. But it seems like a bad practice nonetheless.
Expected behavior
Use
fs.readFile
. The enclosing function is already async anyway.Additional context
JS SDK v3 has the same behavior. So apologies if the SDK is doing something magic and the I/O isn't actually sync. https://github.com/aws/aws-sdk-js-v3/blob/5e0a46a9f4a35cdb200f7eccef09fb4c6ad76e9c/packages/credential-provider-web-identity/src/fromTokenFile.ts#L38
The text was updated successfully, but these errors were encountered: