Skip to content
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.

Sign up for GitHub

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

Add option for clearing global normalClient dialer #5807

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Add option for clearing global normalClient dialer #5807

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04b447c
Add option for clear global `normalClient` dialer
yaron12n Nov 10, 2024
29dc70b
Merge remote-tracking branch 'origin' into pr/5807
ehsandeep Dec 19, 2024
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
8 changes: 8 additions & 0 deletions pkg/protocols/network/networkclientpool/clientpool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package networkclientpool

import (
"github.com/projectdiscovery/fastdialer/fastdialer"

"github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/protocolstate"
"github.com/projectdiscovery/nuclei/v3/pkg/types"
)
Expand Down Expand Up @@ -32,3 +33,10 @@ func (c *Configuration) Hash() string {
func Get(options *types.Options, configuration *Configuration /*TODO review unused parameters*/) (*fastdialer.Dialer, error) {
return normalClient, nil
}

// Clear clears cached client pool implementation
func Clear() {
Comment on lines +37 to +38
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance documentation and add tests

The documentation for the Clear function is minimal and there are no tests demonstrating its proper usage and thread-safety.

Please add:

  1. Comprehensive documentation explaining:
    • When to use Clear
    • Thread-safety considerations
    • Potential impacts on existing connections
  2. Tests covering:
    • Basic Clear functionality
    • Concurrent Init/Clear operations
    • Resource cleanup verification

Would you like me to help create a test suite for this functionality?

if normalClient != nil {
normalClient = nil
}
}
Comment on lines +37 to +42
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance the Clear function implementation

The current implementation has several potential issues:

  1. No synchronization protection
  2. No proper cleanup of resources
  3. No handling of in-flight operations

Consider implementing a more robust clearing mechanism:

func Clear() {
+	normalClientMutex.Lock()
+	defer normalClientMutex.Unlock()
+
	if normalClient != nil {
+		// Close any existing connections
+		if closer, ok := normalClient.GetDialer().(io.Closer); ok {
+			_ = closer.Close()
+		}
		normalClient = nil
	}
}

Committable suggestion skipped: line range outside the PR's diff.