diff --git a/module/modmanager/manager.go b/module/modmanager/manager.go index f5b05478f6e..92fcaff5c35 100644 --- a/module/modmanager/manager.go +++ b/module/modmanager/manager.go @@ -8,7 +8,6 @@ import ( "os" "path/filepath" "slices" - "strconv" "strings" "sync" "sync/atomic" @@ -43,9 +42,6 @@ import ( rutils "go.viam.com/rdk/utils" ) -// tcpPortRange is the beginning of the port range. Only used when ViamTCPSockets() = true. -const tcpPortRange = 13500 - var ( validateConfigTimeout = 5 * time.Second errMessageExitStatus143 = "exit status 143" @@ -75,7 +71,6 @@ func NewManager( packagesDir: options.PackagesDir, ftdc: options.FTDC, } - ret.nextPort.Store(tcpPortRange) return ret } @@ -109,8 +104,6 @@ type module struct { inRecoveryLock sync.Mutex logger logging.Logger ftdc *ftdc.FTDC - // port stores the listen port of this module when ViamTCPSockets() = true. - port int } type addedResource struct { @@ -191,8 +184,6 @@ type Manager struct { restartCtx context.Context restartCtxCancel context.CancelFunc ftdc *ftdc.FTDC - // nextPort manages ports when ViamTCPSockets() = true. - nextPort atomic.Int32 } // Close terminates module connections and processes. @@ -350,7 +341,6 @@ func (mgr *Manager) add(ctx context.Context, conf config.Module, moduleLogger lo resources: map[resource.Name]*addedResource{}, logger: moduleLogger, ftdc: mgr.ftdc, - port: int(mgr.nextPort.Add(1)), } if err := mgr.startModule(ctx, mod); err != nil { @@ -1025,10 +1015,10 @@ func (mgr *Manager) attemptRestart(ctx context.Context, mod *module) []resource. func (m *module) dial() error { // TODO(PRODUCT-343): session support probably means interceptors here var err error - addrToDial := m.addr - if !rutils.TCPRegex.MatchString(addrToDial) { - addrToDial = "unix://" + m.addr - } + // Unix socket paths on Windows are formatted with "C:" and backslashes (\), which are incompatible + // with url.Parse(). Reformat the path to use forward slashes (/) and remove the "C:" prefix for compatibility. + addrToDial := "unix://" + m.addr + conn, err := grpc.Dial( //nolint:staticcheck addrToDial, grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(rpc.MaxMessageSize)), @@ -1139,15 +1129,11 @@ func (m *module) startProcess( ) error { var err error - if rutils.ViamTCPSockets() { - m.addr = "127.0.0.1:" + strconv.Itoa(m.port) - } else { - // append a random alpha string to the module name while creating a socket address to avoid conflicts - // with old versions of the module. - if m.addr, err = modlib.CreateSocketAddress( - filepath.Dir(parentAddr), fmt.Sprintf("%s-%s", m.cfg.Name, utils.RandomAlphaString(5))); err != nil { - return err - } + // append a random alpha string to the module name while creating a socket address to avoid conflicts + // with old versions of the module. + if m.addr, err = modlib.CreateSocketAddress( + filepath.Dir(parentAddr), fmt.Sprintf("%s-%s", m.cfg.Name, utils.RandomAlphaString(5))); err != nil { + return err } // We evaluate the Module's ExePath absolutely in the viam-server process so that @@ -1215,15 +1201,12 @@ func (m *module) startProcess( ) } } - if !rutils.TCPRegex.MatchString(m.addr) { - // note: we don't do this check in TCP mode because TCP addresses are not file paths and will fail check. - err = modlib.CheckSocketOwner(m.addr) - if errors.Is(err, fs.ErrNotExist) { - continue - } - if err != nil { - return errors.WithMessage(err, "module startup failed") - } + err = modlib.CheckSocketOwner(m.addr) + if errors.Is(err, fs.ErrNotExist) { + continue + } + if err != nil { + return errors.WithMessage(err, "module startup failed") } break } diff --git a/module/modmanager/manager_test.go b/module/modmanager/manager_test.go index 5ab749732f6..917aea210b5 100644 --- a/module/modmanager/manager_test.go +++ b/module/modmanager/manager_test.go @@ -10,7 +10,6 @@ import ( "os/exec" "path/filepath" "runtime" - "strconv" "sync/atomic" "syscall" "testing" @@ -37,7 +36,6 @@ import ( modmanageroptions "go.viam.com/rdk/module/modmanager/options" "go.viam.com/rdk/module/modmaninterface" "go.viam.com/rdk/resource" - "go.viam.com/rdk/robot/web" rtestutils "go.viam.com/rdk/testutils" rutils "go.viam.com/rdk/utils" ) @@ -49,12 +47,8 @@ func setupSocketWithRobot(t *testing.T) string { var socketAddress string var err error - if rutils.ViamTCPSockets() { - socketAddress = "127.0.0.1:" + strconv.Itoa(web.TestTCPParentPort) - } else { - socketAddress, err = modlib.CreateSocketAddress(t.TempDir(), "parent") - test.That(t, err, test.ShouldBeNil) - } + socketAddress, err = modlib.CreateSocketAddress(t.TempDir(), "parent") + test.That(t, err, test.ShouldBeNil) rtestutils.MakeRobotForModuleLogging(t, socketAddress) return socketAddress @@ -136,7 +130,6 @@ func TestModManagerFunctions(t *testing.T) { }, dataDir: "module-data-dir", logger: logger, - port: tcpPortRange, } err = mod.startProcess(ctx, parentAddr, nil, viamHomeTemp, filepath.Join(viamHomeTemp, "packages")) diff --git a/module/module.go b/module/module.go index 877741ec0da..ed2f027366d 100644 --- a/module/module.go +++ b/module/module.go @@ -97,7 +97,18 @@ func CreateSocketAddress(parentDir, desiredName string) (string, error) { // Assemble the truncated socket address socketHashSuffix := desiredNameHash[:socketHashSuffixLength] truncatedName := desiredName[:(numRemainingChars - socketHashSuffixLength - 1)] - return filepath.Join(baseAddr, fmt.Sprintf("%s-%s%s", truncatedName, socketHashSuffix, socketSuffix)), nil + addr := strings.Replace( + strings.ReplaceAll( + filepath.Join( + baseAddr, + fmt.Sprintf("%s-%s%s", truncatedName, socketHashSuffix, socketSuffix)), + `\`, + `/`), + "C:", + "", + 1, + ) + return addr, nil } // HandlerMap is the format for api->model pairs that the module will service. @@ -283,9 +294,6 @@ func (m *Module) Start(ctx context.Context) error { var lis net.Listener prot := "unix" - if rutils.TCPRegex.MatchString(m.addr) { - prot = "tcp" - } if err := MakeSelfOwnedFilesFunc(func() error { var err error lis, err = net.Listen(prot, m.addr) @@ -355,13 +363,10 @@ func (m *Module) connectParent(ctx context.Context) error { return nil } - fullAddr := m.parentAddr - if !rutils.TCPRegex.MatchString(m.parentAddr) { - if err := CheckSocketOwner(m.parentAddr); err != nil { - return err - } - fullAddr = "unix://" + m.parentAddr + if err := CheckSocketOwner(m.parentAddr); err != nil { + return err } + fullAddr := "unix://" + m.parentAddr // moduleLoggers may be creating the client connection below, so use a // different logger here to avoid a deadlock where the client connection diff --git a/robot/web/web.go b/robot/web/web.go index dbf5ae4a713..2eb93e8c869 100644 --- a/robot/web/web.go +++ b/robot/web/web.go @@ -12,7 +12,6 @@ import ( "net/url" "os" "path/filepath" - "strconv" "strings" "sync" "sync/atomic" @@ -168,17 +167,11 @@ func (svc *webService) StartModule(ctx context.Context) error { if err != nil { return errors.WithMessage(err, "module startup failed") } - - if rutils.ViamTCPSockets() { - addr = "127.0.0.1:" + strconv.Itoa(TCPParentPort) - lis, err = net.Listen("tcp", addr) - } else { - addr, err = module.CreateSocketAddress(dir, "parent") - if err != nil { - return errors.WithMessage(err, "module startup failed") - } - lis, err = net.Listen("unix", addr) + addr, err = module.CreateSocketAddress(dir, "parent") + if err != nil { + return errors.WithMessage(err, "module startup failed") } + lis, err = net.Listen("unix", addr) if err != nil { return errors.WithMessage(err, "failed to listen") } diff --git a/testutils/module_utils.go b/testutils/module_utils.go index bc62ac88509..a9403d98c89 100644 --- a/testutils/module_utils.go +++ b/testutils/module_utils.go @@ -10,7 +10,6 @@ import ( "go.viam.com/utils/rpc" "go.viam.com/rdk/logging" - "go.viam.com/rdk/utils" ) type mockRobotService struct { @@ -27,9 +26,6 @@ func (ms *mockRobotService) Log(ctx context.Context, req *robotpb.LogRequest) (* func MakeRobotForModuleLogging(t *testing.T, parentAddr string) rpc.Server { logger := logging.NewTestLogger(t) prot := "unix" - if utils.TCPRegex.MatchString(parentAddr) { - prot = "tcp" - } listener, err := net.Listen(prot, parentAddr) test.That(t, err, test.ShouldBeNil) rpcServer, err := rpc.NewServer(logger, rpc.WithUnauthenticated()) diff --git a/utils/env.go b/utils/env.go index 0d872ad9287..05f185fd793 100644 --- a/utils/env.go +++ b/utils/env.go @@ -2,9 +2,7 @@ package utils import ( "os" - "regexp" "runtime" - "slices" "time" "go.viam.com/rdk/logging" @@ -35,9 +33,6 @@ const ( // EnvTrueValues contains strings that we interpret as boolean true in env vars. var EnvTrueValues = []string{"true", "yes", "1", "TRUE", "YES"} -// TCPRegex tests whether a module address is TCP (vs unix sockets). See also ViamTCPSockets(). -var TCPRegex = regexp.MustCompile(`:\d+$`) - // GetResourceConfigurationTimeout calculates the resource configuration // timeout (env variable value if set, DefaultResourceConfigurationTimeout // otherwise). @@ -86,12 +81,3 @@ func PlatformMkdirTemp(dir, pattern string) (string, error) { } return os.MkdirTemp(dir, pattern) } - -// ViamTCPSockets returns true if an env is set or if the platform requires it. -func ViamTCPSockets() bool { - // note: unix sockets have been supported on windows for a while, but go-grpc does not support them. - // 2017 support announcement: https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ - // go grpc client bug on win: https://github.com/dotnet/aspnetcore/issues/47043 - return runtime.GOOS == "windows" || - slices.Contains(EnvTrueValues, os.Getenv("VIAM_TCP_SOCKETS")) -}