fix(portforward): rework run loop and fix deadlocks (#1874)

Author: qdm12

cmd/gluetun/main.go

@@ -377,9 +377,7 @@ func _main(ctx context.Context, buildInfo models.BuildInformation,
 	portForwardLogger := logger.New(log.SetComponent("port forwarding"))
 	portForwardLooper := portforward.NewLoop(allSettings.VPN.Provider.PortForwarding,
 		httpClient, firewallConf, portForwardLogger, puid, pgid)
-	portForwardHandler, portForwardCtx, portForwardDone := goshutdown.NewGoRoutineHandler(
-		"port forwarding", goroutine.OptionTimeout(time.Second))
-	go portForwardLooper.Run(portForwardCtx, portForwardDone)
+	portForwardRunError, _ := portForwardLooper.Start(context.Background())
 
 	unboundLogger := logger.New(log.SetComponent("dns"))
 	unboundLooper := dns.NewLoop(dnsConf, allSettings.DNS, httpClient,
@@ -481,13 +479,21 @@ func _main(ctx context.Context, buildInfo models.BuildInformation,
 		order.OptionOnSuccess(defaultShutdownOnSuccess),
 		order.OptionOnFailure(defaultShutdownOnFailure))
 	orderHandler.Append(controlGroupHandler, tickersGroupHandler, healthServerHandler,
-		vpnHandler, portForwardHandler, otherGroupHandler)
+		vpnHandler, otherGroupHandler)
 
 	// Start VPN for the first time in a blocking call
 	// until the VPN is launched
 	_, _ = vpnLooper.ApplyStatus(ctx, constants.Running) // TODO option to disable with variable
 
-	<-ctx.Done()
+	select {
+	case <-ctx.Done():
+		err = portForwardLooper.Stop()
+		if err != nil {
+			logger.Error("stopping port forward loop: " + err.Error())
+		}
+	case err := <-portForwardRunError:
+		logger.Errorf("port forwarding loop crashed: %s", err)
+	}
 
 	return orderHandler.Shutdown(context.Background())
 }

internal/configuration/settings/portforward.go

@@ -30,7 +30,7 @@ type PortForwarding struct {
 	Filepath *string `json:"status_file_path"`
 }
 
-func (p PortForwarding) validate(vpnProvider string) (err error) {
+func (p PortForwarding) Validate(vpnProvider string) (err error) {
 	if !*p.Enabled {
 		return nil
 	}
@@ -59,7 +59,7 @@ func (p PortForwarding) validate(vpnProvider string) (err error) {
 	return nil
 }
 
-func (p *PortForwarding) copy() (copied PortForwarding) {
+func (p *PortForwarding) Copy() (copied PortForwarding) {
 	return PortForwarding{
 		Enabled:  gosettings.CopyPointer(p.Enabled),
 		Provider: gosettings.CopyPointer(p.Provider),
@@ -73,7 +73,7 @@ func (p *PortForwarding) mergeWith(other PortForwarding) {
 	p.Filepath = gosettings.MergeWithPointer(p.Filepath, other.Filepath)
 }
 
-func (p *PortForwarding) overrideWith(other PortForwarding) {
+func (p *PortForwarding) OverrideWith(other PortForwarding) {
 	p.Enabled = gosettings.OverrideWithPointer(p.Enabled, other.Enabled)
 	p.Provider = gosettings.OverrideWithPointer(p.Provider, other.Provider)
 	p.Filepath = gosettings.OverrideWithPointer(p.Filepath, other.Filepath)

internal/configuration/settings/provider.go

@@ -49,7 +49,7 @@ func (p *Provider) validate(vpnType string, storage Storage) (err error) {
 		return fmt.Errorf("server selection: %w", err)
 	}
 
-	err = p.PortForwarding.validate(*p.Name)
+	err = p.PortForwarding.Validate(*p.Name)
 	if err != nil {
 		return fmt.Errorf("port forwarding: %w", err)
 	}
@@ -61,7 +61,7 @@ func (p *Provider) copy() (copied Provider) {
 	return Provider{
 		Name:            gosettings.CopyPointer(p.Name),
 		ServerSelection: p.ServerSelection.copy(),
-		PortForwarding:  p.PortForwarding.copy(),
+		PortForwarding:  p.PortForwarding.Copy(),
 	}
 }
 
@@ -74,7 +74,7 @@ func (p *Provider) mergeWith(other Provider) {
 func (p *Provider) overrideWith(other Provider) {
 	p.Name = gosettings.OverrideWithPointer(p.Name, other.Name)
 	p.ServerSelection.overrideWith(other.ServerSelection)
-	p.PortForwarding.overrideWith(other.PortForwarding)
+	p.PortForwarding.OverrideWith(other.PortForwarding)
 }
 
 func (p *Provider) setDefaults() {

internal/portforward/firewall.go

@@ -1,10 +1,20 @@
 package portforward
 
-import (
-	"context"
-)
+import "context"
+
+type Service interface {
+	Start(ctx context.Context) (runError <-chan error, err error)
+	Stop() (err error)
+	GetPortForwarded() (port uint16)
+}
 
 type PortAllower interface {
 	SetAllowedPort(ctx context.Context, port uint16, intf string) (err error)
 	RemoveAllowedPort(ctx context.Context, port uint16) (err error)
 }
+
+type Logger interface {
+	Info(s string)
+	Warn(s string)
+	Error(s string)
+}