From 778ef18158d5d7dd49035a75ed8fee996f6551cb Mon Sep 17 00:00:00 2001 From: Integralist Date: Tue, 7 Nov 2023 17:49:27 +0000 Subject: [PATCH 1/3] refactor(compute/serve): replace log.Fatal usage with channel --- pkg/commands/compute/serve.go | 51 +++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/pkg/commands/compute/serve.go b/pkg/commands/compute/serve.go index caee27a12..71ad36bf4 100644 --- a/pkg/commands/compute/serve.go +++ b/pkg/commands/compute/serve.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "io/fs" - "log" "net" "net/url" "os" @@ -613,6 +612,7 @@ func local(opts localOpts) error { } s.MonitorSignals() + failure := make(chan error) restart := make(chan bool) if opts.watch { root := "." @@ -625,14 +625,14 @@ func local(opts localOpts) error { } gi := ignoreFiles(opts.watchDir) - go watchFiles(root, gi, opts.verbose, s, opts.out, restart) + go watchFiles(root, gi, opts.verbose, s, opts.out, restart, failure) } - // NOTE: Once we run the viceroy executable, then it can be stopped by one of - // two separate mechanisms: + // NOTE: The viceroy executable can be stopped by one of three mechanisms. // // 1. File modification // 2. Explicit signal (SIGINT, SIGTERM etc). + // 3. Irrecoverable error (i.e. error watching files). // // In the case of a signal (e.g. user presses Ctrl-c) the listener logic // inside of (*fstexec.Streaming).MonitorSignals() will call @@ -648,6 +648,25 @@ func local(opts localOpts) error { // we do finally come to stop the `serve` command (e.g. user presses Ctrl-c). // How big an issue this is depends on how many file modifications a user // makes, because having lots of signal listeners could exhaust resources. + // + // When there is an error setting up the watching of files, if we error we + // need to signal the error using a channel as watching files happens + // asynchronously in a goroutine. We also need to be able to signal the + // viceroy process to be killed, and we do that using `s.Signal(os.Kill)` from + // within the relevant error handling blocks in `watchFiles`, where upon the + // below `select` statement will pull the error message from the `failure` + // channel and return it to the user. If we fail to kill the Viceroy process + // then we still want to pull an error from the `failure` channel and so we + // have a separate `select` statement to check for any initial errors prior to + // the Viceroy executable starting and an error occurring in `watchFiles`. + select { + case channelErr := <-failure: + s.SignalCh <- syscall.SIGTERM + return channelErr + case <-time.After(1 * time.Second): + // no-op: allow logic to flow to starting up Viceroy executable. + } + if err := s.Exec(); err != nil { if !strings.Contains(err.Error(), "signal: ") { opts.errLog.Add(err) @@ -658,6 +677,9 @@ func local(opts localOpts) error { } if strings.Contains(e, "killed") { select { + case channelErr := <-failure: + s.SignalCh <- syscall.SIGTERM + return channelErr case <-restart: s.SignalCh <- syscall.SIGTERM return fsterr.ErrViceroyRestart @@ -673,10 +695,16 @@ func local(opts localOpts) error { // watchFiles watches the language source directory and restarts the viceroy // executable when changes are detected. -func watchFiles(root string, gi *ignore.GitIgnore, verbose bool, s *fstexec.Streaming, out io.Writer, restart chan<- bool) { +func watchFiles(root string, gi *ignore.GitIgnore, verbose bool, s *fstexec.Streaming, out io.Writer, restart chan<- bool, failure chan<- error) { watcher, err := fsnotify.NewWatcher() if err != nil { - log.Fatal(err) + signalErr := s.Signal(os.Kill) + if signalErr != nil { + failure <- fmt.Errorf("failed to stop Viceroy executable while trying to create a fsnotify.Watcher: %w: %w", signalErr, err) + return + } + failure <- fmt.Errorf("failed to create a fsnotify.Watcher: %w", err) + return } defer watcher.Close() @@ -727,7 +755,8 @@ func watchFiles(root string, gi *ignore.GitIgnore, verbose bool, s *fstexec.Stre // and is a low risk concern. err := s.Signal(os.Kill) if err != nil { - log.Fatal(err) + failure <- fmt.Errorf("failed to stop Viceroy executable while trying to restart the process: %w", err) + return } restart <- true @@ -775,7 +804,13 @@ func watchFiles(root string, gi *ignore.GitIgnore, verbose bool, s *fstexec.Stre return nil }) if err != nil { - log.Fatal(err) + signalErr := s.Signal(os.Kill) + if signalErr != nil { + failure <- fmt.Errorf("failed to stop Viceroy executable while trying to walk directory tree for watching files: %w: %w", signalErr, err) + return + } + failure <- fmt.Errorf("failed to walk directory tree for watching files: %w", err) + return } if verbose { From 8f3f5a7d3af45e8a65239fc0d3916dfa3f4253a5 Mon Sep 17 00:00:00 2001 From: Integralist Date: Wed, 8 Nov 2023 10:29:38 +0000 Subject: [PATCH 2/3] doc(compute/init): correct method annotation --- pkg/commands/compute/init.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/commands/compute/init.go b/pkg/commands/compute/init.go index e1393d2db..67ac88588 100644 --- a/pkg/commands/compute/init.go +++ b/pkg/commands/compute/init.go @@ -552,7 +552,7 @@ func promptPackageAuthors(flags global.Flags, authors []string, manifestEmail st return authors, nil } -// promptForLanguage prompts the user for a package language unless already +// PromptForLanguage prompts the user for a package language unless already // defined either via the corresponding CLI flag or the manifest file. func (c *InitCommand) PromptForLanguage(languages []*Language, in io.Reader, out io.Writer) (*Language, error) { var ( @@ -871,7 +871,7 @@ mimes: return spinner.Stop() } -// clonePackageFromEndpoint clones the given repo (from) into a temp directory, +// ClonePackageFromEndpoint clones the given repo (from) into a temp directory, // then copies specific files to the destination directory (path). func (c *InitCommand) ClonePackageFromEndpoint(branch, tag string) error { from := c.cloneFrom From e6a6c6219ee56e05886b1e3b8e19cd58ca26d694 Mon Sep 17 00:00:00 2001 From: Integralist Date: Wed, 8 Nov 2023 10:38:33 +0000 Subject: [PATCH 3/3] refactor(compute/serve): rename error variable --- pkg/commands/compute/serve.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/commands/compute/serve.go b/pkg/commands/compute/serve.go index 71ad36bf4..9cc9045cf 100644 --- a/pkg/commands/compute/serve.go +++ b/pkg/commands/compute/serve.go @@ -660,9 +660,9 @@ func local(opts localOpts) error { // have a separate `select` statement to check for any initial errors prior to // the Viceroy executable starting and an error occurring in `watchFiles`. select { - case channelErr := <-failure: + case asyncErr := <-failure: s.SignalCh <- syscall.SIGTERM - return channelErr + return asyncErr case <-time.After(1 * time.Second): // no-op: allow logic to flow to starting up Viceroy executable. } @@ -677,9 +677,9 @@ func local(opts localOpts) error { } if strings.Contains(e, "killed") { select { - case channelErr := <-failure: + case asyncErr := <-failure: s.SignalCh <- syscall.SIGTERM - return channelErr + return asyncErr case <-restart: s.SignalCh <- syscall.SIGTERM return fsterr.ErrViceroyRestart