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

indexer: Ensure declared module calls get decoded #1395

Merged
merged 4 commits into from
Sep 11, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 8, 2023

Related: hashicorp/terraform-schema#254

The PR is not strictly hard dependency for this one but it is helpful in testing, to verify that the race condition is no longer present.


Context

Some time ago, we discovered a race condition, which may not have been as well understood at the time. We needed to make sure that go-to-definition and go-to-references works on first load for module inputs.

Module inputs are represented as reference origins and as with everything else, the attribute schema is what instructs the job collecting the reference origins how and whether to collect those origins. Obtaining the schema for modules is basically the job of LoadModuleMetadata - in particular we generate the schema based on parsed variables & outputs.

Prior to this PR, we never processed "module calls" directly from didOpen or didChange and only relied on the walker to get to them by the time we needed them. This would usually work fine if no documents were open at the time of initial walking. Often times though the walking may take a lot of time for larger workspaces and users may open files very early. This meant submodule data was unavailable when the user opened the module which calls those submodules, making the reference origin collection job "clue-less" about those module inputs.

We worked around that race condition by inferring the inputs - basically assuming all declared inputs have their corresponding variable declarations. As with most workarounds, this eventually caught up with us 😅 . In particular, validation (as implemented in #1368) was also acting based on this inferred (potentially incorrect) schema and worse - the inferred schema was assumed to be relevant for all modules with the same source.

Therefore, hashicorp/terraform-schema#254 removes the workaround and this PR addresses the race condition.

Technically this may result in more jobs getting scheduled but the overall performance impact should be relatively minimal because:

  • it only impacts open files/modules and the assumption is that users generally don't open all modules that are in their workspace at the same time
  • the jobs exit early if the module data is already available

Testing

I am frankly not sure how to best test this (in code) as the problem is usually only visible with high enough number of jobs / large workspaces.

I did test it manually in one of our large repositories and can confirm the "go-to-definition" works cca 10 out of 10 times on continuous reload, whereas previously it would end up broken on every ~5th reload.

@radeksimko radeksimko marked this pull request as ready for review September 8, 2023 12:29
@radeksimko radeksimko requested a review from a team as a code owner September 8, 2023 12:29
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

👍 Nice work. I see only a very, very small performance impact, which is definitely negligible.

@radeksimko radeksimko merged commit 5ccfde6 into main Sep 11, 2023
21 checks passed
@radeksimko radeksimko deleted the b-fix-mod-calls-race-condition branch September 11, 2023 13:06
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
@xiehan xiehan added this to the v0.32.0 (tentative) milestone Jan 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working technical-debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants