Ok so in this test we have two port visibilities:

Lines 52 to 61 in 7924878

    
           portVisibilities := []portVisibility{ 
        
           	{ 
        
           		number:     9999, 
        
           		visibility: "public", 
        
           	}, 
        
           	{ 
        
           		number:     80, 
        
           		visibility: "org", 
        
           	}, 
        
           }

We collect these and then pass them to the app UpdatePortVisibility method:

cli/pkg/cmd/codespace/ports_test.go

Lines 81 to 88 in 7924878

    
           var portArgs []string 
        
           for _, pv := range portVisibilities { 
        
           	portArgs = append(portArgs, fmt.Sprintf("%d:%s", pv.number, pv.visibility)) 
        
           } 
        
           selector := &CodespaceSelector{api: a.apiClient, codespaceName: "codespace-name"} 
        
           return a.UpdatePortVisibility(ctx, selector, portArgs)

This creates a port forwarder using a shared codespace connection on which it calls UpdatePortVisibility for each of the original ports:

https://github.com/cli/cli/blob/trunk/pkg/cmd/codespace/ports.go#L240-L257

A codespace connection wraps a tunnelClient:

cli/internal/codespaces/connection/connection.go

Lines 62 to 65 in 7924878

    
           tunnelClient, err := getTunnelClient(ctx, tunnelManager, tunnel, options) 
        
           if err != nil { 
        
           	return nil, fmt.Errorf("error getting tunnel client: %w", err) 
        
           }

Within UpdatePortVisibility we create a new goroutine that calls Connect on the tunnel client and defers a CloseSSHConnection (which behind the scenes calls Close on the tunnelClient:

cli/internal/codespaces/portforwarder/port_forwarder.go

Lines 258 to 267 in 7924878

    
           go func() { 
        
           	// Connect to the tunnel 
        
           	err = fwd.connection.TunnelClient.Connect(ctx, "") 
        
           	if err != nil { 
        
           		done <- fmt.Errorf("connect failed: %v", err) 
        
           		return 
        
           	} 
        
           	// Close the SSH connection when we're done 
        
           	defer fwd.CloseSSHConnection()

Since we now have a goroutine per UpdatePortVisibility this means that they are concurrently using the tunnelClient which as shown in the previous comment is not thread safe over the ssh field.

This is not just a race in the tests but also the implementation and should be fixed there. I don't think I have enough context to make a recommendation on how to resolve this so I will put this down now and leave it to @dmgardiner25 to take ownership of.

	portVisibilities := []portVisibility{
	{
	number: 9999,
	visibility: "public",
	},
	{
	number: 80,
	visibility: "org",
	},
	}

	var portArgs []string
	for _, pv := range portVisibilities {
	portArgs = append(portArgs, fmt.Sprintf("%d:%s", pv.number, pv.visibility))
	}

	selector := &CodespaceSelector{api: a.apiClient, codespaceName: "codespace-name"}

	return a.UpdatePortVisibility(ctx, selector, portArgs)

	tunnelClient, err := getTunnelClient(ctx, tunnelManager, tunnel, options)
	if err != nil {
	return nil, fmt.Errorf("error getting tunnel client: %w", err)
	}

	go func() {
	// Connect to the tunnel
	err = fwd.connection.TunnelClient.Connect(ctx, "")
	if err != nil {
	done <- fmt.Errorf("connect failed: %v", err)
	return
	}

	// Close the SSH connection when we're done
	defer fwd.CloseSSHConnection()

Data race when running tests #8266

Description

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions