Skip to content

Migrate to signal.NotifyContext #145

Description

@perk11

Here's what that migration concretely looks like, grounded in this codebase.

  1. What signal.NotifyContext replaces at the top

The current setup is hand-rolled:

  exit := make(chan os.Signal, 1)                                                                                                                                                                                                                                                                                                                    
  signal.Notify(exit, os.Interrupt, syscall.SIGTERM, syscall.SIGINT)                                                                                                                                                                                                                                                                                 
  // ...                                                                                                                                                                                                                                                                                                                                             
  for {                                                                                                                                                                                                                                                                                                                                              
      receivedSignal := <-exit                                                                                                                                                                                                                                                                                                                       
      interrupted.Store(true)                                                                                                                                                                                                                                                                                                                        
      ...                                                                                                                                                                                                                                                                                                                                            
  }                                                                                                                                                                                                                                                                                                                                                  

signal.NotifyContext (Go 1.16+) collapses the first three lines into one and gives you a context that cancels on signal automatically:

  ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM, syscall.SIGINT)                                                                                                                                                                                                                                             
  defer stop()                                                                                                                                                                                                                                                                                                                                       

The package-global interrupted atomic.Bool then goes away entirely. Every interrupted.Load() becomes ctx.Err() != nil (or a helper like isInterrupted(ctx)).

  1. The teardown sequence itself stays

This is the part context doesn't simplify. After the signal, the code does a synchronous, hard teardown:

  resourceManager.serviceMutex.Lock()                                                                                                                                                                                                                                                                                                                
  for name := range resourceManager.runningServices {                                                                                                                                                                                                                                                                                                
      stopService(*findServiceConfigByName(name))                                                                                                                                                                                                                                                                                                    
  }                                                                                                                                                                                                                                                                                                                                                  
  log.Printf("Done, exiting")                                                                                                                                                                                                                                                                                                                        
  os.Exit(0)                                                                                                                                                                                                                                                                                                                                         

Context cancellation is cooperative — it just flips a flag and closes a channel. It doesn't run your cleanup or force a stop. You still need this explicit "lock, stop everything, exit" block to guarantee all child processes are dead before the process dies. So the shutdown path shrinks by one line (no interrupted.Store(true); the context
cancel does it) but the structure is unchanged. Anyone expecting context to make teardown "automatic" will be disappointed — that's not what it buys you here.

  1. The real change: threading ctx through the call graph

This is the bulk of the work. Every function that reads interrupted gains a ctx context.Context parameter, and so do all its callers:

┌─────────────────────────────────────────────────┬────────────────────────────────────────────┬───────────────────────────────────────────────────────┐
│ Function (line) │ Currently reads │ Becomes │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ startProxy (488) │ accept loop guard │ ctx param, or close listener on <-ctx.Done() (see §4) │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ handleConnection (540) │ early-return guard │ ctx param │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ startServiceIfNotAlreadyRunningAndConnect (612) │ 2 guards │ ctx param │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ startService (656) │ 3 guards │ ctx param │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ performHealthCheck (795) │ polling-loop guard │ ctx param → can use net.Dialer.DialContext │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ tryConnectingUntilTimeoutOrProcessExit (901) │ 1 guard │ ctx param → DialContext │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ stopService (1506) │ 2 guards (TryLock branch + cleanup branch) │ ctx param │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ monitorProcess (1573) │ 2 guards │ ctx param │
├─────────────────────────────────────────────────┼────────────────────────────────────────────┼───────────────────────────────────────────────────────┤
│ maybeGetRunningService (79) │ 1 guard │ needs ctx or stays on a separate mechanism │
└─────────────────────────────────────────────────┴────────────────────────────────────────────┴───────────────────────────────────────────────────────┘

That's ~9 functions plus their callers (the main goroutine launcher, the go handleConnection(...) spawns, etc.), and each of those functions is exercised by tests. It's a wide, mechanical-but-behavior-sensitive diff — which is exactly why it doesn't belong in the same commit as the -race unblock.

  1. The accept-loop subtlety

listener.Accept() blocks. Today it's guarded by checking the flag at the top of the loop, but that only works because after interrupt the process is about to os.Exit anyway — a blocked Accept doesn't matter. With a context you have a cleaner, non-exit-dependent option: spawn a goroutine that does <-ctx.Done(); listener.Close(), which
makes the blocked Accept return an error immediately. That's the idiomatic shape, but it's a behavioral change worth its own test (currently the accept loop doesn't error out cleanly on signal; it relies on os.Exit).

  1. The clientDisconnected unification opportunity

Worth flagging because it's the part that actually improves the design rather than just restyling it. Several functions already thread a clientDisconnected <-chan struct{}:

  func tryConnectingUntilTimeoutOrProcessExit(..., clientDisconnected <-chan struct{}) (net.Conn, bool)                                                                                                                                                                                                                                              
  func reserveResources(..., clientDisconnected <-chan struct{}) bool                                                                                                                                                                                                                                                                                
  func startService(..., clientDisconnected <-chan struct{}) (net.Conn, error)                                                                                                                                                                                                                                                                       
  func performHealthCheck(..., clientDisconnected <-chan struct{}) error                                                                                                                                                                                                                                                                             

That's a hand-rolled, single-shot context. In a context-based world those become child contexts (ctx, cancel := context.WithCancel(parent)), and client-disconnect and process-interrupt compose — a function waiting on "either the client left or the whole process is shutting down" just selects on one ctx.Done() instead of two channels. The
poll loops in performHealthCheck/tryConnectingUntilTimeoutOrProcessExit can replace their manual select { case <-clientDisconnected: } with ctx, cancel := context.WithTimeout(...) plus net.Dialer.DialContext, which also gets them proper deadline semantics. That's a genuine readability and correctness win, not just cosmetics.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions