From b7ce9e91c8e670be3ea2b933ca8345b228c17b8e Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Tue, 16 Nov 2021 22:31:13 +0100 Subject: [PATCH 1/4] make all operations async by default --- internal/langserver/handlers/did_change.go | 18 ++++++------------ internal/langserver/handlers/did_open.go | 11 ++++++----- internal/terraform/module/module_manager.go | 9 --------- internal/terraform/module/types.go | 1 - internal/terraform/module/watcher.go | 3 +-- 5 files changed, 13 insertions(+), 29 deletions(-) diff --git a/internal/langserver/handlers/did_change.go b/internal/langserver/handlers/did_change.go index da4122c8..8a6e4384 100644 --- a/internal/langserver/handlers/did_change.go +++ b/internal/langserver/handlers/did_change.go @@ -61,39 +61,33 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument return err } - err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseModuleConfiguration) + err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseModuleConfiguration, nil) if err != nil { return err } - err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseVariables) + err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseVariables, nil) if err != nil { return err } - // TODO: parallelise the operations below in a workgroup - err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeLoadModuleMetadata) + err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeLoadModuleMetadata, nil) if err != nil { return err } - err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceTargets) + err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceTargets, nil) if err != nil { return err } - err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceOrigins) + err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceOrigins, nil) if err != nil { return err } + // TODO notifier, err := lsctx.DiagnosticsNotifier(ctx) if err != nil { return err } - // obtain fresh module state after the above operations finished - mod, err = modMgr.ModuleByPath(fh.Dir()) - if err != nil { - return err - } - diags := diagnostics.NewDiagnostics() diags.EmptyRootDiagnostic() diags.Append("HCL", mod.ModuleDiagnostics.AsMap()) diff --git a/internal/langserver/handlers/did_open.go b/internal/langserver/handlers/did_open.go index b5146533..0af8b404 100644 --- a/internal/langserver/handlers/did_open.go +++ b/internal/langserver/handlers/did_open.go @@ -48,11 +48,11 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe // We reparse because the file being opened may not match // (originally parsed) content on the disk // TODO: Do this only if we can verify the file differs? - modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseModuleConfiguration) - modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseVariables) - modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeLoadModuleMetadata) - modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceTargets) - modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceOrigins) + modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseModuleConfiguration, nil) + modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseVariables, nil) + modMgr.EnqueueModuleOp(mod.Path, op.OpTypeLoadModuleMetadata, nil) + modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceTargets, nil) + modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceOrigins, nil) if mod.TerraformVersionState == op.OpStateUnknown { modMgr.EnqueueModuleOp(mod.Path, op.OpTypeGetTerraformVersion, nil) @@ -70,6 +70,7 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe } } + // TODO: move notifier, err := lsctx.DiagnosticsNotifier(ctx) if err != nil { return err diff --git a/internal/terraform/module/module_manager.go b/internal/terraform/module/module_manager.go index f3645fe2..ef643078 100644 --- a/internal/terraform/module/module_manager.go +++ b/internal/terraform/module/module_manager.go @@ -81,15 +81,6 @@ func (mm *moduleManager) RemoveModule(modPath string) error { return mm.moduleStore.Remove(modPath) } -func (mm *moduleManager) EnqueueModuleOpWait(modPath string, opType op.OpType) error { - modOp := NewModuleOperation(modPath, opType) - mm.loader.EnqueueModuleOp(modOp) - - <-modOp.Done() - - return nil -} - func (mm *moduleManager) EnqueueModuleOp(modPath string, opType op.OpType, deferFunc DeferFunc) error { modOp := NewModuleOperation(modPath, opType) modOp.Defer = deferFunc diff --git a/internal/terraform/module/types.go b/internal/terraform/module/types.go index 156521f0..ec284e0e 100644 --- a/internal/terraform/module/types.go +++ b/internal/terraform/module/types.go @@ -36,7 +36,6 @@ type ModuleManager interface { AddModule(modPath string) (Module, error) RemoveModule(modPath string) error EnqueueModuleOp(modPath string, opType op.OpType, deferFunc DeferFunc) error - EnqueueModuleOpWait(modPath string, opType op.OpType) error CancelLoading() } diff --git a/internal/terraform/module/watcher.go b/internal/terraform/module/watcher.go index 657e26f8..1ad49388 100644 --- a/internal/terraform/module/watcher.go +++ b/internal/terraform/module/watcher.go @@ -245,8 +245,7 @@ func decodeCalledModulesFunc(modMgr ModuleManager, w Watcher, modPath string) De } modMgr.AddModule(mc.Path) - modMgr.EnqueueModuleOpWait(mc.Path, op.OpTypeParseModuleConfiguration) - + modMgr.EnqueueModuleOp(mc.Path, op.OpTypeParseModuleConfiguration, nil) modMgr.EnqueueModuleOp(mc.Path, op.OpTypeParseVariables, nil) modMgr.EnqueueModuleOp(mc.Path, op.OpTypeLoadModuleMetadata, nil) modMgr.EnqueueModuleOp(mc.Path, op.OpTypeDecodeReferenceTargets, nil) From bc5d16d423e84b66e3db6c89cec0b4c661fb1873 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 17 Nov 2021 11:22:55 +0100 Subject: [PATCH 2/4] decouple diagnostics from handlers into a hook --- .../langserver/diagnostics/diagnostics.go | 19 +++++++------ .../diagnostics/diagnostics_test.go | 24 +++++++++------- internal/langserver/handlers/did_change.go | 17 ----------- internal/langserver/handlers/did_close.go | 17 ----------- internal/langserver/handlers/did_open.go | 18 ------------ internal/langserver/handlers/hooks_module.go | 28 +++++++++++++++++++ internal/langserver/handlers/initialize.go | 4 ++- internal/langserver/handlers/service.go | 14 +++++----- internal/langserver/session/types.go | 9 ++++++ internal/state/module.go | 14 ++++++++-- 10 files changed, 84 insertions(+), 80 deletions(-) diff --git a/internal/langserver/diagnostics/diagnostics.go b/internal/langserver/diagnostics/diagnostics.go index 795b6e3b..7c2eef35 100644 --- a/internal/langserver/diagnostics/diagnostics.go +++ b/internal/langserver/diagnostics/diagnostics.go @@ -6,7 +6,6 @@ import ( "path/filepath" "sync" - "github.com/creachadair/jrpc2" "github.com/hashicorp/hcl/v2" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" @@ -21,20 +20,24 @@ type diagContext struct { type DiagnosticSource string +type ClientNotifier interface { + Notify(ctx context.Context, method string, params interface{}) error +} + // Notifier is a type responsible for queueing HCL diagnostics to be converted // and sent to the client type Notifier struct { logger *log.Logger - sessCtx context.Context diags chan diagContext + clientNotifier ClientNotifier closeDiagsOnce sync.Once } -func NewNotifier(sessCtx context.Context, logger *log.Logger) *Notifier { +func NewNotifier(clientNotifier ClientNotifier, logger *log.Logger) *Notifier { n := &Notifier{ - logger: logger, - sessCtx: sessCtx, - diags: make(chan diagContext, 50), + logger: logger, + diags: make(chan diagContext, 50), + clientNotifier: clientNotifier, } go n.notify() return n @@ -44,7 +47,7 @@ func NewNotifier(sessCtx context.Context, logger *log.Logger) *Notifier { // A dir path is passed which is joined with the filename keys of the map, to form a file URI. func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags Diagnostics) { select { - case <-n.sessCtx.Done(): + case <-ctx.Done(): n.closeDiagsOnce.Do(func() { close(n.diags) }) @@ -68,7 +71,7 @@ func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags Di func (n *Notifier) notify() { for d := range n.diags { - if err := jrpc2.ServerFromContext(d.ctx).Notify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ + if err := n.clientNotifier.Notify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ URI: d.uri, Diagnostics: d.diags, }); err != nil { diff --git a/internal/langserver/diagnostics/diagnostics_test.go b/internal/langserver/diagnostics/diagnostics_test.go index 1d5beffd..33dfb46f 100644 --- a/internal/langserver/diagnostics/diagnostics_test.go +++ b/internal/langserver/diagnostics/diagnostics_test.go @@ -13,10 +13,7 @@ import ( var discardLogger = log.New(ioutil.Discard, "", 0) func TestDiags_Closes(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - n := NewNotifier(ctx, discardLogger) - - cancel() + n := NewNotifier(noopNotifier{}, discardLogger) diags := NewDiagnostics() diags.Append("test", map[string]hcl.Diagnostics{ @@ -27,7 +24,9 @@ func TestDiags_Closes(t *testing.T) { }, }) - n.PublishHCLDiags(context.Background(), t.TempDir(), diags) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + n.PublishHCLDiags(ctx, t.TempDir(), diags) if _, open := <-n.diags; open { t.Fatal("channel should be closed") @@ -41,10 +40,7 @@ func TestPublish_DoesNotSendAfterClose(t *testing.T) { } }() - ctx, cancel := context.WithCancel(context.Background()) - n := NewNotifier(ctx, discardLogger) - - cancel() + n := NewNotifier(noopNotifier{}, discardLogger) diags := NewDiagnostics() diags.Append("test", map[string]hcl.Diagnostics{ @@ -55,7 +51,9 @@ func TestPublish_DoesNotSendAfterClose(t *testing.T) { }, }) - n.PublishHCLDiags(context.Background(), t.TempDir(), diags) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + n.PublishHCLDiags(ctx, t.TempDir(), diags) } func TestDiagnostics_Append(t *testing.T) { @@ -133,3 +131,9 @@ func TestDiagnostics_Append(t *testing.T) { t.Fatalf("diagnostics mismatch: %s", diff) } } + +type noopNotifier struct{} + +func (noopNotifier) Notify(ctx context.Context, method string, params interface{}) error { + return nil +} diff --git a/internal/langserver/handlers/did_change.go b/internal/langserver/handlers/did_change.go index 8a6e4384..495ce4a1 100644 --- a/internal/langserver/handlers/did_change.go +++ b/internal/langserver/handlers/did_change.go @@ -5,10 +5,8 @@ import ( "fmt" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" op "github.com/hashicorp/terraform-ls/internal/terraform/module/operation" ) @@ -82,20 +80,5 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument return err } - // TODO - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", mod.ModuleDiagnostics.AsMap()) - diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap()) - if vf, ok := ast.NewVarsFilename(f.Filename()); ok && !vf.IsAutoloaded() { - diags.Append("HCL", mod.VarsDiagnostics.ForFile(vf).AsMap()) - } - notifier.PublishHCLDiags(ctx, mod.Path, diags) - return nil } diff --git a/internal/langserver/handlers/did_close.go b/internal/langserver/handlers/did_close.go index 019fd35c..8b4bb52d 100644 --- a/internal/langserver/handlers/did_close.go +++ b/internal/langserver/handlers/did_close.go @@ -3,12 +3,9 @@ package handlers import ( "context" - "github.com/hashicorp/hcl/v2" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" ) func TextDocumentDidClose(ctx context.Context, params lsp.DidCloseTextDocumentParams) error { @@ -23,19 +20,5 @@ func TextDocumentDidClose(ctx context.Context, params lsp.DidCloseTextDocumentPa return err } - if vf, ok := ast.NewVarsFilename(fh.Filename()); ok && !vf.IsAutoloaded() { - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", map[string]hcl.Diagnostics{ - fh.Filename(): {}, - }) - notifier.PublishHCLDiags(ctx, fh.Dir(), diags) - } - return nil } diff --git a/internal/langserver/handlers/did_open.go b/internal/langserver/handlers/did_open.go index 0af8b404..a324d6f6 100644 --- a/internal/langserver/handlers/did_open.go +++ b/internal/langserver/handlers/did_open.go @@ -4,10 +4,8 @@ import ( "context" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" "github.com/hashicorp/terraform-ls/internal/terraform/module" op "github.com/hashicorp/terraform-ls/internal/terraform/module/operation" ) @@ -70,21 +68,5 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe } } - // TODO: move - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", mod.ModuleDiagnostics.AsMap()) - diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap()) - if vf, ok := ast.NewVarsFilename(f.Filename()); ok && !vf.IsAutoloaded() { - diags.Append("HCL", mod.VarsDiagnostics.ForFile(vf).AsMap()) - } - - notifier.PublishHCLDiags(ctx, mod.Path, diags) - return nil } diff --git a/internal/langserver/handlers/hooks_module.go b/internal/langserver/handlers/hooks_module.go index b0ae8c4d..4e61b3d1 100644 --- a/internal/langserver/handlers/hooks_module.go +++ b/internal/langserver/handlers/hooks_module.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" "github.com/hashicorp/terraform-ls/internal/state" "github.com/hashicorp/terraform-ls/internal/telemetry" "github.com/hashicorp/terraform-schema/backend" @@ -91,3 +92,30 @@ func sendModuleTelemetry(ctx context.Context, store *state.StateStore, telemetry telemetrySender.SendEvent(ctx, "moduleData", properties) } } + +func updateDiagnostics(ctx context.Context, notifier *diagnostics.Notifier) state.ModuleChangeHook { + return func(oldMod, newMod *state.Module) { + // TODO: check if diagnostics have actually changed + oldDiags, newDiags := 0, 0 + if oldMod != nil { + oldDiags = len(oldMod.ModuleDiagnostics) + len(oldMod.VarsDiagnostics) + } + if newMod != nil { + newDiags = len(newMod.ModuleDiagnostics) + len(newMod.VarsDiagnostics) + } + + diags := diagnostics.NewDiagnostics() + diags.EmptyRootDiagnostic() + + defer notifier.PublishHCLDiags(ctx, newMod.Path, diags) + + if oldDiags == 0 && newDiags == 0 { + return + } + + if newMod != nil { + diags.Append("HCL", newMod.ModuleDiagnostics.AsMap()) + diags.Append("HCL", newMod.VarsDiagnostics.AutoloadedOnly().AsMap()) + } + } +} diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 74520d17..a08166b6 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -70,9 +70,11 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) expClientCaps := lsp.ExperimentalClientCapabilities(clientCaps.Experimental) + svc.server = jrpc2.ServerFromContext(ctx) + if tv, ok := expClientCaps.TelemetryVersion(); ok { svc.logger.Printf("enabling telemetry (version: %d)", tv) - err := svc.setupTelemetry(tv, jrpc2.ServerFromContext(ctx)) + err := svc.setupTelemetry(tv, svc.server) if err != nil { svc.logger.Printf("failed to setup telemetry: %s", err) } diff --git a/internal/langserver/handlers/service.go b/internal/langserver/handlers/service.go index 33eaa521..606a6132 100644 --- a/internal/langserver/handlers/service.go +++ b/internal/langserver/handlers/service.go @@ -52,6 +52,8 @@ type service struct { telemetry telemetry.Sender decoder *decoder.Decoder stateStore *state.StateStore + server session.Server + diagsNotifier *diagnostics.Notifier additionalHandlers map[string]rpch.Func } @@ -100,8 +102,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { lh := LogHandler(svc.logger) cc := &lsp.ClientCapabilities{} - notifier := diagnostics.NewNotifier(svc.sessCtx, svc.logger) - rootDir := "" commandPrefix := "" clientName := "" @@ -140,7 +140,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) ctx = lsctx.WithDocumentStorage(ctx, svc.fs) ctx = lsctx.WithModuleManager(ctx, svc.modMgr) return handle(ctx, req, TextDocumentDidChange) @@ -150,7 +149,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) ctx = lsctx.WithDocumentStorage(ctx, svc.fs) ctx = lsctx.WithModuleManager(ctx, svc.modMgr) ctx = lsctx.WithWatcher(ctx, svc.watcher) @@ -161,7 +159,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) ctx = lsctx.WithDocumentStorage(ctx, svc.fs) return handle(ctx, req, TextDocumentDidClose) }, @@ -287,7 +284,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) + ctx = lsctx.WithDiagnosticsNotifier(ctx, svc.diagsNotifier) ctx = lsctx.WithExperimentalFeatures(ctx, &expFeatures) ctx = lsctx.WithModuleFinder(ctx, svc.modMgr) ctx = exec.WithExecutorOpts(ctx, svc.tfExecOpts) @@ -328,7 +325,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { ctx = lsctx.WithModuleWalker(ctx, svc.walker) ctx = lsctx.WithWatcher(ctx, svc.watcher) ctx = lsctx.WithRootDirectory(ctx, &rootDir) - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) + ctx = lsctx.WithDiagnosticsNotifier(ctx, svc.diagsNotifier) ctx = exec.WithExecutorOpts(ctx, svc.tfExecOpts) ctx = exec.WithExecutorFactory(ctx, svc.tfExecFactory) @@ -430,6 +427,8 @@ func (svc *service) configureSessionDependencies(ctx context.Context, cfgOpts *s execOpts.Timeout = d } + svc.diagsNotifier = diagnostics.NewNotifier(svc.server, svc.logger) + svc.tfExecOpts = execOpts svc.sessCtx = exec.WithExecutorOpts(svc.sessCtx, execOpts) @@ -445,6 +444,7 @@ func (svc *service) configureSessionDependencies(ctx context.Context, cfgOpts *s svc.stateStore.SetLogger(svc.logger) svc.stateStore.Modules.ChangeHooks = state.ModuleChangeHooks{ + updateDiagnostics(svc.sessCtx, svc.diagsNotifier), sendModuleTelemetry(svc.sessCtx, svc.stateStore, svc.telemetry), } diff --git a/internal/langserver/session/types.go b/internal/langserver/session/types.go index 194cab8e..6d129ea7 100644 --- a/internal/langserver/session/types.go +++ b/internal/langserver/session/types.go @@ -18,3 +18,12 @@ type ClientNotifier interface { } type SessionFactory func(context.Context) Session + +type ClientCaller interface { + Callback(ctx context.Context, method string, params interface{}) (*jrpc2.Response, error) +} + +type Server interface { + ClientNotifier + ClientCaller +} diff --git a/internal/state/module.go b/internal/state/module.go index ea55df57..c10fcea5 100644 --- a/internal/state/module.go +++ b/internal/state/module.go @@ -699,11 +699,12 @@ func (s *ModuleStore) UpdateModuleDiagnostics(path string, diags ast.ModDiags) e txn := s.db.Txn(true) defer txn.Abort() - mod, err := moduleCopyByPath(txn, path) + oldMod, err := moduleByPath(txn, path) if err != nil { return err } + mod := oldMod.Copy() mod.ModuleDiagnostics = diags err = txn.Insert(s.tableName, mod) @@ -711,6 +712,10 @@ func (s *ModuleStore) UpdateModuleDiagnostics(path string, diags ast.ModDiags) e return err } + txn.Defer(func() { + go s.ChangeHooks.notifyModuleChange(oldMod, mod) + }) + txn.Commit() return nil } @@ -719,11 +724,12 @@ func (s *ModuleStore) UpdateVarsDiagnostics(path string, diags ast.VarsDiags) er txn := s.db.Txn(true) defer txn.Abort() - mod, err := moduleCopyByPath(txn, path) + oldMod, err := moduleByPath(txn, path) if err != nil { return err } + mod := oldMod.Copy() mod.VarsDiagnostics = diags err = txn.Insert(s.tableName, mod) @@ -731,6 +737,10 @@ func (s *ModuleStore) UpdateVarsDiagnostics(path string, diags ast.VarsDiags) er return err } + txn.Defer(func() { + go s.ChangeHooks.notifyModuleChange(oldMod, mod) + }) + txn.Commit() return nil } From 806088e8f4da68c6b88bcdb876447ec8cba17d55 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 17 Nov 2021 12:59:48 +0100 Subject: [PATCH 3/4] Prevent future deadlocks When a (1) capped channel has more than one consumer, one of them will effectively remain blocking as the other consumes the message from the channel. Closing the channel unblocks any/all consumers. Here we also unpublish previously public ModuleOperation.Done() to further prevent deadlocks in the future, i.e. effectively to prevent any place that schedules tasks to wait. The only reason we keep the channel there for now is to make testing *within the package* easier. --- .../terraform/module/module_loader_test.go | 19 +++++++------------ internal/terraform/module/module_manager.go | 2 +- internal/terraform/module/module_ops.go | 3 ++- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/internal/terraform/module/module_loader_test.go b/internal/terraform/module/module_loader_test.go index 1ff9e5e5..fb6b5fdf 100644 --- a/internal/terraform/module/module_loader_test.go +++ b/internal/terraform/module/module_loader_test.go @@ -46,7 +46,7 @@ func TestModuleLoader_referenceCollection(t *testing.T) { if err != nil { t.Fatal(err) } - <-modOp.doneCh + <-modOp.done() manifestOp := NewModuleOperation(modPath, op.OpTypeLoadModuleMetadata) err = ml.EnqueueModuleOp(manifestOp) @@ -66,17 +66,12 @@ func TestModuleLoader_referenceCollection(t *testing.T) { } t.Log("waiting for all operations to finish") - for i := 0; i <= 2; i++ { - select { - case <-manifestOp.doneCh: - t.Log("manifest parsed") - case <-originsOp.doneCh: - t.Log("origins collected") - case <-targetsOp.doneCh: - t.Log("targets collected") - case <-ctx.Done(): - } - } + <-manifestOp.done() + t.Log("manifest parsed") + <-originsOp.done() + t.Log("origins collected") + <-targetsOp.done() + t.Log("targets collected") mod, err := ss.Modules.ModuleByPath(modPath) if err != nil { diff --git a/internal/terraform/module/module_manager.go b/internal/terraform/module/module_manager.go index ef643078..9938bd8d 100644 --- a/internal/terraform/module/module_manager.go +++ b/internal/terraform/module/module_manager.go @@ -86,7 +86,7 @@ func (mm *moduleManager) EnqueueModuleOp(modPath string, opType op.OpType, defer modOp.Defer = deferFunc mm.loader.EnqueueModuleOp(modOp) if mm.syncLoading { - <-modOp.Done() + <-modOp.done() } return nil } diff --git a/internal/terraform/module/module_ops.go b/internal/terraform/module/module_ops.go index 83d0bd74..47690e40 100644 --- a/internal/terraform/module/module_ops.go +++ b/internal/terraform/module/module_ops.go @@ -39,9 +39,10 @@ func NewModuleOperation(modPath string, typ op.OpType) ModuleOperation { func (mo ModuleOperation) markAsDone() { mo.doneCh <- struct{}{} + close(mo.doneCh) } -func (mo ModuleOperation) Done() <-chan struct{} { +func (mo ModuleOperation) done() <-chan struct{} { return mo.doneCh } From 76111123ed7ed9a81d0d8a54a18db6ddd8d418ea Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 17 Nov 2021 15:09:12 +0100 Subject: [PATCH 4/4] Parse walked module Previously we would (mistakenly) not parse module in walker, only when it's opened (as part of didOpen or didChange). This fixes that to help publish diagnostics from all modules at startup time. --- internal/terraform/module/walker.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/terraform/module/walker.go b/internal/terraform/module/walker.go index 7e892539..fced5100 100644 --- a/internal/terraform/module/walker.go +++ b/internal/terraform/module/walker.go @@ -260,6 +260,11 @@ func (w *Walker) walk(ctx context.Context, rootPath string) error { } } + err = w.modMgr.EnqueueModuleOp(dir, op.OpTypeParseModuleConfiguration, nil) + if err != nil { + return err + } + err = w.modMgr.EnqueueModuleOp(dir, op.OpTypeGetTerraformVersion, nil) if err != nil { return err