TokenFileWebIdentityCredentials uses sync I/O to load credential

[gabegorelick]

• [x] I've gone through Developer Guide and API reference
• [x] I've checked AWS Forums and StackOverflow for answers
• [x] I've searched for previous similar issues and didn't find any solution

Describe the bug

TokenFileWebIdentityCredentials.load, which is called by refresh(), calls fs.readFileSync. This presumably blocks the event loop.

https://github.com/aws/aws-sdk-js/blob/33a46b88db36247aa4c0bbe7bc8abad4a38afdeb/lib/credentials/token_file_web_identity_credentials.js#L163

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

[alexforsyth]

@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.

[gabegorelick]

The file is only being loaded once.

I haven't checked to see whether the results of the read are cached. If you're correct and readFileSync is only called once, then that certainly minimizes the impact of this issue.

Out of curiosity what is the impact of this issue right now for you?

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 readFileSync call.

https://nodejs.org/en/docs/guides/blocking-vs-non-blocking/#concurrency-and-throughput:

As an example, let's consider a case where each request to a web server takes 50ms to complete and 45ms of that 50ms is database I/O that can be done asynchronously. Choosing non-blocking asynchronous operations frees up that 45ms per request to handle other requests. This is a significant difference in capacity just by choosing to use non-blocking methods instead of blocking methods.

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.

[vudh1]

Hi @gabegorelick changing this to feature-request issue since it is not a bug. Feel free to open a PR to fix this.

[gabegorelick]

FileSystemCredentials also suffers from a similar issue.

[github-actions[bot]]

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.