-
Notifications
You must be signed in to change notification settings - Fork 5
perf: adjust read/write timeouts for redir #561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 { | ||||||
|
@@ -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) | ||||||
|
@@ -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. | ||||||
|
||||||
|
@@ -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
|
||||||
func (t *Target) setReadDeadlineIfNeeded() error { | ||||||
t.deadlineMutex.Lock() | ||||||
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 = defaultReadTimeout / 2 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
|
||||||
if t.lastReadDeadlineSet.IsZero() || now.Sub(t.lastReadDeadlineSet) > deadlineRefreshInterval { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. Positive FeedbackNegative Feedback |
||||||
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
|
||||||
} | ||||||
|
||||||
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
|
||||||
func (t *Target) setWriteDeadlineIfNeeded() error { | ||||||
t.deadlineMutex.Lock() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
|
||||||
if t.lastWriteDeadlineSet.IsZero() || now.Sub(t.lastWriteDeadlineSet) > deadlineRefreshInterval { | ||||||
newDeadline := now.Add(defaultWriteTimeout) | ||||||
if err := t.conn.SetWriteDeadline(newDeadline); err != nil { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The write deadline function calls Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
return err | ||||||
} | ||||||
t.lastWriteDeadlineSet = now | ||||||
Check failure on line 207 in pkg/wsman/client/wsman_tcp.go
|
||||||
} | ||||||
|
||||||
return nil | ||||||
} | ||||||
|
||||||
// CloseConnection cleanly closes the TCP connection. | ||||||
func (t *Target) CloseConnection() error { | ||||||
if t.conn == nil { | ||||||
|
@@ -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 | ||||||
} |
There was a problem hiding this comment.
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.