Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/wsman/client/wsman.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ type Target struct {
UseTLS bool
InsecureSkipVerify bool
PinnedCert string
tlsConfig *tls.Config
tlsConfig *tls.Config
lastReadDeadlineSet time.Time // Track when read deadline was last set
lastWriteDeadlineSet time.Time // Track when write deadline was last set
deadlineMutex sync.Mutex // Protect deadline setting operations
}

const timeout = 10 * time.Second
Expand Down
64 changes: 64 additions & 0 deletions pkg/wsman/client/wsman_tcp.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@

// defaultKeepAlive configures TCP keepalive probe interval on the dialer.
defaultKeepAlive = 30 * time.Second

// defaultReadTimeout sets the maximum time to wait for read operations.
defaultReadTimeout = 30 * time.Second

// defaultWriteTimeout sets the maximum time to wait for write operations.
defaultWriteTimeout = 30 * time.Second
)

func NewWsmanTCP(cp Parameters) *Target {
Expand Down Expand Up @@ -127,6 +133,11 @@
return fmt.Errorf("no active connection")
}

// Optimize: Only set deadline if enough time has passed or it's not set
if err := t.setWriteDeadlineIfNeeded(); err != nil {
return fmt.Errorf("failed to set write deadline: %w", err)
}

_, err := t.conn.Write(data)
if err != nil {
return fmt.Errorf("failed to send data: %w", err)
Expand All @@ -141,6 +152,11 @@
return nil, fmt.Errorf("no active connection")
}

// Optimize: Only set deadline if enough time has passed or it's not set
if err := t.setReadDeadlineIfNeeded(); err != nil {
return nil, fmt.Errorf("failed to set read deadline: %w", err)
}

tmp := t.bufferPool.Get().([]byte)
defer t.bufferPool.Put(tmp) //nolint:staticcheck // changing the argument to be pointer-like to avoid allocations caused issues.

Expand All @@ -152,6 +168,48 @@
return append([]byte(nil), tmp[:n]...), nil
}

// setReadDeadlineIfNeeded sets read deadline only when needed to reduce syscall overhead

Check failure on line 171 in pkg/wsman/client/wsman_tcp.go

View workflow job for this annotation

GitHub Actions / runner / golangci-lint

[golangci] reported by reviewdog 🐶 Comment should end in a period (godot) Raw Output: pkg/wsman/client/wsman_tcp.go:171:1: Comment should end in a period (godot) // setReadDeadlineIfNeeded sets read deadline only when needed to reduce syscall overhead ^
func (t *Target) setReadDeadlineIfNeeded() error {
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deadline optimization could create a race condition where multiple goroutines might call Send/Receive simultaneously. Consider if the current mutex protection is sufficient for all concurrent access patterns.

Copilot uses AI. Check for mistakes.

t.deadlineMutex.Lock()
defer t.deadlineMutex.Unlock()

Check failure on line 175 in pkg/wsman/client/wsman_tcp.go

View workflow job for this annotation

GitHub Actions / runner / golangci-lint

[golangci] reported by reviewdog 🐶 File is not properly formatted (gci) Raw Output: pkg/wsman/client/wsman_tcp.go:175:1: File is not properly formatted (gci) ^
now := time.Now()
// Only set deadline if it's been more than half the timeout period since last set
// or if it's never been set (zero value)
const deadlineRefreshInterval = defaultReadTimeout / 2
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic number division by 2 should be extracted to a named constant to improve code clarity and maintainability.

Suggested change
const deadlineRefreshInterval = defaultReadTimeout / 2
const deadlineRefreshInterval = defaultReadTimeout / deadlineRefreshDivisor

Copilot uses AI. Check for mistakes.


if t.lastReadDeadlineSet.IsZero() || now.Sub(t.lastReadDeadlineSet) > deadlineRefreshInterval {
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling time.Now() on every Send/Receive operation may impact performance. Consider caching the current time or using a less precise but faster time source for the deadline refresh logic.

Copilot uses AI. Check for mistakes.

newDeadline := now.Add(defaultReadTimeout)
if err := t.conn.SetReadDeadline(newDeadline); err != nil {
return err
}
t.lastReadDeadlineSet = now

Check failure on line 186 in pkg/wsman/client/wsman_tcp.go

View workflow job for this annotation

GitHub Actions / runner / golangci-lint

[golangci] reported by reviewdog 🐶 assignments should only be cuddled with other assignments (wsl) Raw Output: pkg/wsman/client/wsman_tcp.go:186:3: assignments should only be cuddled with other assignments (wsl) t.lastReadDeadlineSet = now ^
}

return nil
}

// setWriteDeadlineIfNeeded sets write deadline only when needed to reduce syscall overhead

Check failure on line 192 in pkg/wsman/client/wsman_tcp.go

View workflow job for this annotation

GitHub Actions / runner / golangci-lint

[golangci] reported by reviewdog 🐶 Comment should end in a period (godot) Raw Output: pkg/wsman/client/wsman_tcp.go:192:1: Comment should end in a period (godot) // setWriteDeadlineIfNeeded sets write deadline only when needed to reduce syscall overhead ^
func (t *Target) setWriteDeadlineIfNeeded() error {
t.deadlineMutex.Lock()
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the same mutex for both read and write deadline operations creates unnecessary contention. Since these operations can be independent, consider using separate mutexes (readDeadlineMutex and writeDeadlineMutex) to allow concurrent read and write deadline updates.

Copilot uses AI. Check for mistakes.

defer t.deadlineMutex.Unlock()

now := time.Now()
// Only set deadline if it's been more than half the timeout period since last set
// or if it's never been set (zero value)
const deadlineRefreshInterval = defaultWriteTimeout / 2
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the same magic number division by 2 from the read method. Consider extracting a shared constant like deadlineRefreshRatio = 0.5 to avoid duplication.

Suggested change
const deadlineRefreshInterval = defaultWriteTimeout / 2
const deadlineRefreshInterval = time.Duration(float64(defaultWriteTimeout) * deadlineRefreshRatio)

Copilot uses AI. Check for mistakes.


if t.lastWriteDeadlineSet.IsZero() || now.Sub(t.lastWriteDeadlineSet) > deadlineRefreshInterval {
newDeadline := now.Add(defaultWriteTimeout)
if err := t.conn.SetWriteDeadline(newDeadline); err != nil {
Copy link
Preview

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The write deadline function calls SetWriteDeadline but updates the shared lastDeadlineSet field, which could interfere with read deadline tracking. This creates a race condition where setting a write deadline could prevent a needed read deadline update.

Copilot uses AI. Check for mistakes.

return err
}
t.lastWriteDeadlineSet = now

Check failure on line 207 in pkg/wsman/client/wsman_tcp.go

View workflow job for this annotation

GitHub Actions / runner / golangci-lint

[golangci] reported by reviewdog 🐶 assignments should only be cuddled with other assignments (wsl) Raw Output: pkg/wsman/client/wsman_tcp.go:207:3: assignments should only be cuddled with other assignments (wsl) t.lastWriteDeadlineSet = now ^ 6 issues: * gci: 2 * godot: 2 * wsl: 2
}

return nil
}

// CloseConnection cleanly closes the TCP connection.
func (t *Target) CloseConnection() error {
if t.conn == nil {
Expand All @@ -164,6 +222,12 @@
}

t.conn = nil

// Reset deadline tracking when connection is closed
t.deadlineMutex.Lock()
t.lastReadDeadlineSet = time.Time{}
t.lastWriteDeadlineSet = time.Time{}
t.deadlineMutex.Unlock()

return nil
}
Loading