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 infinite loop when fetching USS resources with stat() #3321

Merged
merged 4 commits into from
Nov 15, 2024

Conversation

benjamin-t-santos
Copy link
Contributor

@benjamin-t-santos benjamin-t-santos commented Nov 14, 2024

Proposed changes

Extenders can fetch a USS resource on demand using vscode.workspace.fs.stat() with the fetch=true query. When done with a USS file that has not expanded in the USS tree view, Zowe Explorer gets stuck in an infinite loop of calling stat() over and over again. Here's how it happens:

  1. vscode.workspace.fs.stat() is called for a USS resource with the fetch query set to true. Example: vscode.workspace.fs.stat(zowe-uss:/zosmf/user/test.txt)
  2. UssFSProvider.remoteLookupForResource() is called to fetch the file from the host.
  3. This eventually calls UssFSProvider.fetchEntries(). fetchEntries() checks if the parent directory of the requested file is in the file system. If it's not, await vscode.workspace.fs.createDirectory(parentUri) is called on line 191 to create it.
  4. stat() is called again for the parentUri with a fetch=true query as a result of calling vscode.workspace.fs.createDirectory(). (I believe this is due to the VS Code file system base implementation. It surprised me but looking at the call stack that is what I see)
  5. UssFSProvider.remoteLookupForResource() is called to fetch the directory from the host. This eventually calls UssFSProvider.fetchEntries()
  6. When listFiles() is called in fetchEntries(), if the resource requested is a directory that does not already exist in the file system, we call vscode.workspace.fs.createDirectory() with the directory's URI. This is the same exact URI that is used when calling createDirectory() in step 3.

Basically, Zowe Explorer keeps calling vscode.workspace.fs.createDirectory() for the parent of the requested file over and over again without ever resolving. This does not happen when using tree views because of how directories are expanded from the top-down. In this case, I as an extender am requesting a file from a parent directory that has not been created.

There are improvements we can make to improve the UssFSProvider for extenders, but with the goal of trying to get a fix for this issue into the v3.0.3 release, I tried to keep my changes minimal. I added code that sets the query of the parentUri to an empty string when calling vscode.workspace.fs.createDirectory() in fetchEntries(). This prevents it from entering the infinite loop by not trying to fetch the requested resource's parent.

This change will not impact the tree view, as those views do not make use of the fetch query. This change will only fix functionality for extenders.

Release Notes

Milestone:

Changelog:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds or improves functionality)
  • Breaking change (a change that would cause existing functionality to not work as expected)
  • Documentation (Markdown, README updates)
  • Other (please specify above in "Proposed changes" section)

Checklist

General

  • I have read the CONTRIBUTOR GUIDANCE wiki
  • All PR dependencies have been merged and published (if applicable)
  • A GIF or screenshot is included in the PR for visual changes
  • The pre-publish command has been executed:
    • v2 and below: yarn workspace vscode-extension-for-zowe vscode:prepublish
    • v3: pnpm --filter vscode-extension-for-zowe vscode:prepublish

Code coverage

  • There is coverage for the code that I have added
  • I have added new test cases and they are passing
  • I have manually tested the changes

Deployment

  • I have added developer documentation (if applicable)
  • Documentation should be added to Zowe Docs
    • If you're an outside contributor, please post in the #zowe-doc Slack channel to coordinate documentation.
    • Otherwise, please check with the rest of the squad about any needed documentation before merging.
  • These changes may need ported to the appropriate branches (list here):

Further comments

Signed-off-by: Benjamin Santos <[email protected]>
Signed-off-by: Benjamin Santos <[email protected]>
Copy link

codecov bot commented Nov 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.99%. Comparing base (f5ca8cc) to head (de97307).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3321   +/-   ##
=======================================
  Coverage   92.99%   92.99%           
=======================================
  Files         116      116           
  Lines       12063    12069    +6     
  Branches     2776     2777    +1     
=======================================
+ Hits        11218    11224    +6     
  Misses        843      843           
  Partials        2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@traeok traeok left a comment

Choose a reason for hiding this comment

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

Thanks for catching this Benjamin, the fix is much appreciated. I tested this with a basic sample extension and the fix looks good to me. Please disregard the failing ze-build (22.x, ubuntu-22.04) check as its unrelated.


It seems that the vscode.workspace.fs wrappers perform some extra checks before creating the directory - my guess is that they probably stat the folder to make sure it doesn't exist before trying to create it. When calling createDirectory directly within the providers, this step doesn't occur, so the infinite loop isn't encountered.

Admittedly, the stat function has caused more grief than expected with the file system provider implementations, especially when it comes to network requests. I also did not expect VS Code to call it as much as it does, and this particular case seems to be a side effect of preserving the fetch query on the original URI 😵‍💫

@benjamin-t-santos
Copy link
Contributor Author

@traeok Thanks for the review. Is it possible to include this fix in the v3.0.3 release?

@JillieBeanSim
Copy link
Contributor

@traeok Thanks for the review. Is it possible to include this fix in the v3.0.3 release?

@zFernand0 @t1m0thyj @adam-wolfe what do y'all think too? I believe we are including a similar fix for another tree.

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @benjamin-t-santos!

@t1m0thyj
Copy link
Member

@zFernand0 @t1m0thyj @adam-wolfe what do y'all think too? I believe we are including a similar fix for another tree.

@JillieBeanSim This fix looks like a simple one that would be good to include 👍

@JillieBeanSim JillieBeanSim merged commit 7143336 into zowe:main Nov 15, 2024
18 of 19 checks passed
@JillieBeanSim
Copy link
Contributor

thanks I will get it ported

JillieBeanSim pushed a commit that referenced this pull request Nov 15, 2024
* remove query when looking up parent

Signed-off-by: Benjamin Santos <[email protected]>

* remove leftover comment

Signed-off-by: Benjamin Santos <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Benjamin Santos <[email protected]>

---------

Signed-off-by: Benjamin Santos <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
@JillieBeanSim JillieBeanSim mentioned this pull request Nov 15, 2024
19 tasks
@JillieBeanSim JillieBeanSim added this to the v3.0.3 milestone Nov 15, 2024
t1m0thyj added a commit that referenced this pull request Nov 15, 2024
Signed-off-by: Timothy Johnson <[email protected]>
zFernand0 added a commit that referenced this pull request Nov 16, 2024
* DatasetFSprovider fetchDataset() - fix behavior with non-existent datasets and members (#3255)

* Fix PDS members in fetchDataset

Signed-off-by: Benjamin Santos <[email protected]>

* Update DatasetFSProvider.unit.test.ts

Signed-off-by: Benjamin Santos <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Benjamin Santos <[email protected]>

* Merge branch 'main' into datasetfsprovider-remote-lookup-fix

Signed-off-by: Benjamin Santos <[email protected]>

* move changelog update

Signed-off-by: Benjamin Santos <[email protected]>

* improve logic

Signed-off-by: Benjamin Santos <[email protected]>

* add ? after checking dsorg

Signed-off-by: Benjamin Santos <[email protected]>

* do not fetch attributes for members

Signed-off-by: Benjamin Santos <[email protected]>

---------

Signed-off-by: Benjamin Santos <[email protected]>

* fix: use `vscode.workspace.fs` for `delete` and `rename` (#3261)

* fix: use vscode.workspace.fs to rename URIs

Signed-off-by: Trae Yelovich <[email protected]>

* chore: update ZE changelog

Signed-off-by: Trae Yelovich <[email protected]>

* update mocks, remaining use of delete/rename

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>

* Fix: To resolve error message upon switching the authentication methods (#3275)

* To handle missing args in basicAuthClearSecureArray

Signed-off-by: Santhoshi Boyina <[email protected]>

* To handle missing args in tokenAuthClearSecureArray

Signed-off-by: Santhoshi Boyina <[email protected]>

* To add unit test case for missing arg's in basicAuthClearSecureArray

Signed-off-by: Santhoshi Boyina <[email protected]>

* To add unit test case for missing arg's in tokenAuthClearSecureArray

Signed-off-by: Santhoshi Boyina <[email protected]>

* To run pre-publish command

Signed-off-by: Santhoshi Boyina <[email protected]>

* To update changelog

Signed-off-by: Santhoshi Boyina <[email protected]>

* To modify tokenAuthClearSecureArray() function

Signed-off-by: Santhoshi Boyina <[email protected]>

* To add additional test cases for tokenAuthClearSecureArray()

Signed-off-by: Santhoshi Boyina <[email protected]>

---------

Signed-off-by: Santhoshi Boyina <[email protected]>

* fix: "Show Config" button in error dialog does not work during initialization (#3274)

* fix: check if profileInfo is nullish during v1 migration

Signed-off-by: Trae Yelovich <[email protected]>

* chore: update ZE changelog

Signed-off-by: Trae Yelovich <[email protected]>

* tests: integration test for broken config

Signed-off-by: Trae Yelovich <[email protected]>

* add unit test for nullish profileinfo

Signed-off-by: Trae Yelovich <[email protected]>

* fix transient failures in UpdateCredentials scenario

Signed-off-by: Trae Yelovich <[email protected]>

* remove extra join import in wdio conf

Signed-off-by: Trae Yelovich <[email protected]>

* make integration test more reliable

Signed-off-by: Trae Yelovich <[email protected]>

* move getprofileinfo call into try/catch during profiles init

Signed-off-by: Trae Yelovich <[email protected]>

* test: open notification center to check for dialog

Signed-off-by: Trae Yelovich <[email protected]>

* add license header to test; add another null check

Signed-off-by: Trae Yelovich <[email protected]>

* add typedoc to ProfilesUtils.getProfileInfo

Signed-off-by: Trae Yelovich <[email protected]>

* setupDefaultCredentialManager: log err msgs, update typedoc

Signed-off-by: Trae Yelovich <[email protected]>

* test: promptUserWithNoConfigs, nullish profileInfo case

Signed-off-by: Trae Yelovich <[email protected]>

* refactor typedoc for setupDefaultCredentialManager

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>

* fix(ds): Call remoteLookupForResource when entry doesn't exist locally (#3268)

Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Co-authored-by: Billie Simmons <[email protected]>

* fix: Only show "No configs detected" prompt if ZE opened (#3281)

* fix: show 'No config detected' prompt once when ZE opened

Signed-off-by: Trae Yelovich <[email protected]>

* chore: update ZE changelog

Signed-off-by: Trae Yelovich <[email protected]>

* resolve failing tests

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: move event into static fn, add coverage

Signed-off-by: Trae Yelovich <[email protected]>

* update changelog

Signed-off-by: Trae Yelovich <[email protected]>

* patch coverage for ProfilesUtils.promptUserWithNoConfigs

Signed-off-by: Trae Yelovich <[email protected]>

* move variable definition

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Co-authored-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>

* fix(v3): Pass `responseTimeout` in z/OSMF MVS and USS API calls (#3292)

* fix: pass responseTimeout to API functions

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: remove fallback for spreading newOptions

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: use optional chaining; work on resolving tests

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: pass profile props to tests, fix types

Signed-off-by: Trae Yelovich <[email protected]>

* tests: resolve failing cases in ZE

Signed-off-by: Trae Yelovich <[email protected]>

* chore: update changelog entry for ZE API

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: remove fallback for spreading undefined options

Signed-off-by: Trae Yelovich <[email protected]>

* fix: add missing functions to MvsApi test list

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>

* run package

Signed-off-by: Billie Simmons <[email protected]>

* chore: remediation commit

Signed-off-by: Fernando Rijo Cedeno <[email protected]>

Third-Party DCO Remediation Commit for benjamin-t-santos <[email protected]>

On behalf of benjamin-t-santos <[email protected]>, I, Fernando Rijo Cedeno <[email protected]>, hereby add my Signed-off-by to this commit: 8957a64

Signed-off-by: Fernando Rijo Cedeno <[email protected]>

Third-Party DCO Remediation Commit for SanthoshiBoyina1 <[email protected]>

On behalf of SanthoshiBoyina1 <[email protected]>, I, Fernando Rijo Cedeno <[email protected]>, hereby add my Signed-off-by to this commit: 86a82f6

Signed-off-by: Fernando Rijo Cedeno <[email protected]>

Signed-off-by: Fernando Rijo Cedeno <[email protected]>

* DatasetFSProvider.readFile() makes at most one API call (#3279)

* fix(ds): Call remoteLookupForResource when entry doesn't exist locally

Signed-off-by: Trae Yelovich <[email protected]>

* readFile() uses one MVS API call

Signed-off-by: Benjamin Santos <[email protected]>

* update unit tests, fix logic of calls

Signed-off-by: Benjamin Santos <[email protected]>

* fix lint error

Signed-off-by: Benjamin Santos <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Benjamin Santos <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Benjamin Santos <[email protected]>
Signed-off-by: benjamin-t-santos <[email protected]>
Co-authored-by: Trae Yelovich <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>

* Change default credentials manager check and not-found dialog (#3297)

* updates to webpack and package.json for dev mode

Signed-off-by: Billie Simmons <[email protected]>

* Remove endless startup loop when default CM cannot be loaded

Signed-off-by: Peter Haumer <[email protected]>

* Initial set of test updates

Signed-off-by: Peter Haumer <[email protected]>

* Merge branch 'main' into change-credentials-manager-check

Signed-off-by: Peter Haumer <[email protected]>

* Convenience launch to only run currently open test

Signed-off-by: Peter Haumer <[email protected]>

* Fixed ProfileUtils tests

Signed-off-by: Peter Haumer <[email protected]>

* Clean up package files

Signed-off-by: Peter Haumer <[email protected]>

* Updated changelog

Signed-off-by: Peter Haumer <[email protected]>

* Reworded info message

Signed-off-by: Peter Haumer <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Fernando Rijo Cedeno <[email protected]>

* Added detail to info message

Signed-off-by: Peter Haumer <[email protected]>

* Updated resource files

Signed-off-by: Peter Haumer <[email protected]>

---------

Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Peter Haumer <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Co-authored-by: Billie Simmons <[email protected]>
Co-authored-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>

* fix(3.0.3): Prompt in editor for 401 error, fix profile propagation (#3318)

* fix: profile change propagation, prompt on auth error

Signed-off-by: Trae Yelovich <[email protected]>

* tests: profile propagation, tree node cases

Signed-off-by: Trae Yelovich <[email protected]>

* chore: changelogs

Signed-off-by: Trae Yelovich <[email protected]>

* test(jobs): add patch coverage

Signed-off-by: Trae Yelovich <[email protected]>

* tests: AuthUtils.promptForAuthError

Signed-off-by: Trae Yelovich <[email protected]>

* tests: UssFSProvider.fetchFileAtUri

Signed-off-by: Trae Yelovich <[email protected]>

* fix: reset wasAccessed flag if ImperativeError caught

Signed-off-by: Trae Yelovich <[email protected]>

* tests: resolve failing USS tests

Signed-off-by: Trae Yelovich <[email protected]>

* fix failing jobs test

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>

* fix(ftp): Generate member name if missing in `putContents` (#3313)

* fix(ftp): Generate member name if missing in putContents

Signed-off-by: Trae Yelovich <[email protected]>

* chore: add entry to FTP changelog

Signed-off-by: Trae Yelovich <[email protected]>

* refactor: remove unused isAbsolutePath import

Signed-off-by: Trae Yelovich <[email protected]>

* chore: undo updates to l10n since no strings were changed

Signed-off-by: Trae Yelovich <[email protected]>

* tests(ftp): PDS upload case, clean up PS upload test

Signed-off-by: Trae Yelovich <[email protected]>

---------

Signed-off-by: Trae Yelovich <[email protected]>
Co-authored-by: Billie Simmons <[email protected]>
Co-authored-by: Fernando Rijo Cedeno <[email protected]>

* run prepublish

Signed-off-by: Billie Simmons <[email protected]>

* Fix infinite loop when fetching USS resources with stat() (#3321)

* remove query when looking up parent

Signed-off-by: Benjamin Santos <[email protected]>

* remove leftover comment

Signed-off-by: Benjamin Santos <[email protected]>

* Update CHANGELOG.md

Signed-off-by: Benjamin Santos <[email protected]>

---------

Signed-off-by: Benjamin Santos <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>

* run prepublish

Signed-off-by: Billie Simmons <[email protected]>

* Fix port of #3321 for 3.0.3 release

Signed-off-by: Timothy Johnson <[email protected]>

---------

Signed-off-by: Benjamin Santos <[email protected]>
Signed-off-by: Trae Yelovich <[email protected]>
Signed-off-by: Santhoshi Boyina <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Billie Simmons <[email protected]>
Signed-off-by: Fernando Rijo Cedeno <[email protected]>
Signed-off-by: benjamin-t-santos <[email protected]>
Signed-off-by: Peter Haumer <[email protected]>
Signed-off-by: Timothy Johnson <[email protected]>
Co-authored-by: benjamin-t-santos <[email protected]>
Co-authored-by: Trae Yelovich <[email protected]>
Co-authored-by: SanthoshiBoyina1 <[email protected]>
Co-authored-by: Fernando Rijo Cedeno <[email protected]>
Co-authored-by: Peter Haumer <[email protected]>
Co-authored-by: Timothy Johnson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

4 participants