Describe the bug
|
// UpdatePortVisibility changes the visibility (private, org, public) of the specified port. |
|
func (fwd *CodespacesPortForwarder) UpdatePortVisibility(ctx context.Context, remotePort int, visibility string) error { |
|
tunnelPort, err := fwd.connection.TunnelManager.GetTunnelPort(ctx, fwd.connection.Tunnel, remotePort, fwd.connection.Options) |
|
if err != nil { |
|
return fmt.Errorf("error getting tunnel port: %w", err) |
|
} |
|
|
|
// If the port visibility isn't changing, don't do anything |
|
if AccessControlEntriesToVisibility(tunnelPort.AccessControl.Entries) == visibility { |
|
return nil |
|
} |
|
|
|
// Delete the existing tunnel port to update |
|
port, err := convertIntToUint16(remotePort) |
|
if err != nil { |
|
return fmt.Errorf("error converting port: %w", err) |
|
} |
|
err = fwd.connection.TunnelManager.DeleteTunnelPort(ctx, fwd.connection.Tunnel, port, fwd.connection.Options) |
|
if err != nil { |
|
return fmt.Errorf("error deleting tunnel port: %w", err) |
|
} |
|
|
|
done := make(chan error) |
|
go func() { |
|
// Connect to the tunnel |
|
err = fwd.connection.Connect(ctx) |
|
if err != nil { |
|
done <- fmt.Errorf("connect failed: %v", err) |
|
return |
|
} |
|
|
|
// Inform the host that we've deleted the port |
|
err = fwd.connection.TunnelClient.RefreshPorts(ctx) |
|
if err != nil { |
|
done <- fmt.Errorf("refresh ports failed: %v", err) |
|
return |
|
} |
|
|
|
// Re-forward the port with the updated visibility |
|
err = fwd.ForwardPort(ctx, ForwardPortOpts{Port: remotePort, Visibility: visibility}) |
|
if err != nil { |
|
done <- fmt.Errorf("error forwarding port: %w", err) |
|
return |
|
} |
|
|
|
done <- nil |
|
}() |
|
|
|
// Wait for the done channel to be closed |
|
select { |
|
case err := <-done: |
|
if err != nil { |
|
// If we fail to re-forward the port, we need to forward again with the original visibility so the port is still accessible |
|
_ = fwd.ForwardPort(ctx, ForwardPortOpts{Port: remotePort, Visibility: AccessControlEntriesToVisibility(tunnelPort.AccessControl.Entries)}) |
|
|
|
return fmt.Errorf("error connecting to tunnel: %w", err) |
|
} |
|
|
|
return nil |
|
case <-ctx.Done(): |
|
return nil |
|
} |
|
} |
|
go func() { |
|
// Connect to the tunnel |
|
err = fwd.connection.Connect(ctx) |
|
if err != nil { |
|
done <- fmt.Errorf("connect failed: %v", err) |
|
return |
|
} |
Two problems:
- assigning
err in go func() defined in the outer scope may cause data race, although now it's protected by done channel synchronization.
err is allocated in heap, with small contribution to GC overhead. But it can be allocated in stack.
Evidence:
run go build -a -gcflags=-m=2 ./... &> build.log for commit 8723e3b, you will see
...
internal/codespaces/portforwarder/port_forwarder.go:256:14: moved to heap: err
internal/codespaces/portforwarder/port_forwarder.go:258:20: ... argument does not escape
internal/codespaces/portforwarder/port_forwarder.go:258:20: &errors.errorString{...} escapes to heap
internal/codespaces/portforwarder/port_forwarder.go:269:20: ... argument does not escape
err is allocated in heap.
Affected version
It seems that commit 581b665 does not solve the race problem.
Steps to reproduce the behavior
It's detected by static analysis through a linter based on CodeQL with escape analysis extension(doc).
Expected vs actual behavior
Expected: In go func, use err := instead of err =
Logs
It's detected by static analysis.