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 da4122c8e..8a6e4384c 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 b51465335..0af8b4046 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 f3645fe20..ef643078b 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 156521f0f..ec284e0e1 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 657e26f83..1ad493885 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 795b6e3b0..7c2eef357 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 1d5beffdd..33dfb46f4 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 8a6e4384c..495ce4a1b 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 019fd35cb..8b4bb52d9 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 0af8b4046..a324d6f64 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 b0ae8c4d0..4e61b3d18 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 74520d17b..a08166b60 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 33eaa5211..606a61320 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 194cab8e2..6d129ea74 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 ea55df570..c10fcea5b 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 1ff9e5e55..fb6b5fdf3 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 ef643078b..9938bd8de 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 83d0bd749..47690e40f 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 7e8925394..fced51005 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