Skip to content

fix: prevent indefinite hang in long-running scans with periodic flush #942

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

Open
Iceshen87 wants to merge 1 commit into
base: main
Choose a base branch
from
+110 −1
Open

fix: prevent indefinite hang in long-running scans with periodic flush #942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 24 additions & 1 deletion pkg/output/file_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ package output
import (
"bufio"
"os"
"sync"
)

// fileWriter is a concurrent file based output writer.
type fileWriter struct {
file *os.File
writer *bufio.Writer
mu sync.Mutex
}

// NewFileOutputWriter creates a new buffered writer for a file
Expand All @@ -22,16 +24,37 @@ func newFileOutputWriter(file string) (*fileWriter, error) {

// WriteString writes an output to the underlying file
func (w *fileWriter) Write(data []byte) error {
w.mu.Lock()
defer w.mu.Unlock()

_, err := w.writer.Write(data)
if err != nil {
return err
}
_, err = w.writer.WriteRune('\n')
return err
if err != nil {
return err
}

// Flush periodically to prevent buffer deadlock in long-running scans
// This fixes issue #819 where tlsx hangs after ~25k targets
err = w.writer.Flush()
if err != nil {
return err
}

// Sync to disk to prevent data loss on crash
//nolint:errcheck // we don't care whether sync failed or succeeded.
w.file.Sync()

Comment on lines +47 to +49
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "file_writer.go" -path "*/output/*" | head -5

Repository: projectdiscovery/tlsx

Length of output: 92

🏁 Script executed:

cat -n ./pkg/output/file_writer.go

Repository: projectdiscovery/tlsx

Length of output: 1898

Propagate Sync() failures instead of suppressing them.

Sync errors are currently ignored on lines 47–49 and 62, so callers can receive nil even when data persistence failed (e.g., disk full / I/O error). Update both the Write() and Close() methods to check and return Sync() errors.

Suggested fix 
 func (w *fileWriter) Write(data []byte) error {
 	w.mu.Lock()
 	defer w.mu.Unlock()
 	
 	_, err := w.writer.Write(data)
 	if err != nil {
 		return err
 	}
 	_, err = w.writer.WriteRune('\n')
 	if err != nil {
 		return err
 	}
 	
 	// Flush periodically to prevent buffer deadlock in long-running scans
 	// This fixes issue `#819` where tlsx hangs after ~25k targets
 	err = w.writer.Flush()
 	if err != nil {
 		return err
 	}
 	
 	// Sync to disk to prevent data loss on crash
-	//nolint:errcheck // we don't care whether sync failed or succeeded.
-	w.file.Sync()
+	if err := w.file.Sync(); err != nil {
+		return err
+	}
 	
 	return nil
 }

 // Close closes the underlying writer flushing everything to disk
 func (w *fileWriter) Close() error {
 	w.mu.Lock()
 	defer w.mu.Unlock()
 	
 	if err := w.writer.Flush(); err != nil {
 		return err
 	}
-	//nolint:errcheck // we don't care whether sync failed or succeeded.
-	w.file.Sync()
-	return w.file.Close()
+	if err := w.file.Sync(); err != nil {
+		return err
+	}
+	return w.file.Close()
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
//nolint:errcheck // we don't care whether sync failed or succeeded.
w.file.Sync()
func (w *fileWriter) Write(data []byte) error {
w.mu.Lock()
defer w.mu.Unlock()
_, err := w.writer.Write(data)
if err != nil {
return err
}
_, err = w.writer.WriteRune('\n')
if err != nil {
return err
}
// Flush periodically to prevent buffer deadlock in long-running scans
// This fixes issue `#819` where tlsx hangs after ~25k targets
err = w.writer.Flush()
if err != nil {
return err
}
// Sync to disk to prevent data loss on crash
if err := w.file.Sync(); err != nil {
return err
}
return nil
}
// Close closes the underlying writer flushing everything to disk
func (w *fileWriter) Close() error {
w.mu.Lock()
defer w.mu.Unlock()
if err := w.writer.Flush(); err != nil {
return err
}
if err := w.file.Sync(); err != nil {
return err
}
return w.file.Close()
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/output/file_writer.go` around lines 47 - 49, The Write and Close methods
in file_writer.go currently ignore errors returned by w.file.Sync(); modify both
functions (Write and Close) to capture the error from w.file.Sync() and return
it to callers instead of suppressing it—i.e., remove the nolint and errcheck
suppression, call if err := w.file.Sync(); err != nil { return err } (or combine
with preexisting errors appropriately) so Sync failures (e.g., disk full/I/O)
propagate; ensure both Write() and Close() return the Sync error when present.

return nil
}

// Close closes the underlying writer flushing everything to disk
func (w *fileWriter) Close() error {
w.mu.Lock()
defer w.mu.Unlock()

if err := w.writer.Flush(); err != nil {
return err
}
Expand Down
86 changes: 86 additions & 0 deletions pkg/output/file_writer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package output

import (
"os"
"testing"
)

func TestFileWriterConcurrent(t *testing.T) {
// Create temporary file
tmpfile, err := os.CreateTemp("", "test-*.jsonl")
if err != nil {
t.Fatal(err)
}
defer os.Remove(tmpfile.Name())

Comment on lines +10 to +15
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n pkg/output/file_writer_test.go

Repository: projectdiscovery/tlsx

Length of output: 2286

🏁 Script executed:

find . -name "*.go" -path "*/output/*" | head -20

Repository: projectdiscovery/tlsx

Length of output: 204

🏁 Script executed:

cat -n pkg/output/file_writer.go

Repository: projectdiscovery/tlsx

Length of output: 1898

Close temp-file descriptors explicitly after CreateTemp.

Both tests keep the tmpfile handle open while newFileOutputWriter() opens the same file with os.Create(), creating multiple simultaneous file handles on the same path. Close tmpfile immediately after capturing the name to avoid descriptor leakage and prevent potential cleanup issues on Windows.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/output/file_writer_test.go` around lines 10 - 15, The temp file created
in the tests (tmpfile from os.CreateTemp) must be closed immediately after
capturing tmpfile.Name() to avoid leaving an open descriptor while
newFileOutputWriter() (which opens the same path with os.Create) opens the file;
update the tests in file_writer_test.go to call tmpfile.Close() right after
storing the name (keeping the defer os.Remove(tmpfile.Name()) for cleanup) so no
duplicate open file handles remain.

writer, err := newFileOutputWriter(tmpfile.Name())
if err != nil {
t.Fatal(err)
}
defer writer.Close()

// Test concurrent writes
done := make(chan bool, 10)
for i := 0; i < 10; i++ {
go func(id int) {
for j := 0; j < 100; j++ {
data := []byte(`{"test": "data"}`)
if err := writer.Write(data); err != nil {
t.Errorf("Write failed: %v", err)
return
}
}
done <- true
}(i)
}

// Wait for all goroutines to complete
for i := 0; i < 10; i++ {
<-done
}

// Verify file was written
info, err := tmpfile.Stat()
if err != nil {
t.Fatal(err)
}

if info.Size() == 0 {
t.Error("Expected file to have content")
}
Comment on lines +23 to +50
Copy link

@coderabbitai coderabbitai bot Mar 5, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "file_writer_test.go" | head -5

Repository: projectdiscovery/tlsx

Length of output: 97

🏁 Script executed:

cat -n pkg/output/file_writer_test.go | head -100

Repository: projectdiscovery/tlsx

Length of output: 2286

🏁 Script executed:

find . -type f -name "*.go" -path "*/output/*" | head -20

Repository: projectdiscovery/tlsx

Length of output: 204

🏁 Script executed:

cat -n pkg/output/file_writer.go

Repository: projectdiscovery/tlsx

Length of output: 1898

TestFileWriterConcurrent can deadlock if writes fail; assertions are too weak.

When writer.Write fails on line 29, the goroutine returns without sending the done signal, causing the main test to hang indefinitely waiting for all 10 receives. Additionally, checking Size() == 0 cannot catch partial-write regressions or data loss.

Use sync.WaitGroup to ensure all goroutines signal completion regardless of error paths, and verify the complete written content against expected line count (each write appends a newline per the implementation).

💡 Suggested fix 
 import (
+	"bytes"
 	"os"
+	"sync"
 	"testing"
 )
@@
-	// Test concurrent writes
-	done := make(chan bool, 10)
-	for i := 0; i < 10; i++ {
-		go func(id int) {
-			for j := 0; j < 100; j++ {
-				data := []byte(`{"test": "data"}`)
-				if err := writer.Write(data); err != nil {
-					t.Errorf("Write failed: %v", err)
-					return
-				}
-			}
-			done <- true
-		}(i)
-	}
-	
-	// Wait for all goroutines to complete
-	for i := 0; i < 10; i++ {
-		<-done
-	}
-	
-	// Verify file was written
-	info, err := tmpfile.Stat()
+	const goroutines = 10
+	const writesPerGoroutine = 100
+	var wg sync.WaitGroup
+	errCh := make(chan error, goroutines*writesPerGoroutine)
+
+	for i := 0; i < goroutines; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			for j := 0; j < writesPerGoroutine; j++ {
+				if err := writer.Write([]byte(`{"test": "data"}`)); err != nil {
+					errCh <- err
+					return
+				}
+			}
+		}()
+	}
+
+	wg.Wait()
+	close(errCh)
+	for err := range errCh {
+		t.Fatalf("Write failed: %v", err)
+	}
+
+	if err := writer.Close(); err != nil {
+		t.Fatal(err)
+	}
+
+	content, err := os.ReadFile(tmpfile.Name())
 	if err != nil {
 		t.Fatal(err)
 	}
-	
-	if info.Size() == 0 {
-		t.Error("Expected file to have content")
+
+	gotLines := bytes.Count(content, []byte("\n"))
+	wantLines := goroutines * writesPerGoroutine
+	if gotLines != wantLines {
+		t.Fatalf("expected %d lines, got %d", wantLines, gotLines)
 	}
 }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/output/file_writer_test.go` around lines 23 - 50, The test
TestFileWriterConcurrent can deadlock because goroutines return on write error
without signaling completion and only checks file size rather than content;
replace the done channel with a sync.WaitGroup in TestFileWriterConcurrent, call
wg.Add(1) per goroutine and defer wg.Done() inside each goroutine so completion
is always signaled even on error, collect any write errors into an errors
channel (or t-safe aggregator) instead of returning early, wait on wg.Wait(),
then read the tmpfile contents (instead of just using tmpfile.Stat/Size) and
verify the number of newline-terminated records matches the expected count
(10*100) to catch partial-write regressions; reference writer.Write and tmpfile
reading in the test when implementing these changes.

}

func TestFileWriterFlush(t *testing.T) {
tmpfile, err := os.CreateTemp("", "test-flush-*.jsonl")
if err != nil {
t.Fatal(err)
}
defer os.Remove(tmpfile.Name())

writer, err := newFileOutputWriter(tmpfile.Name())
if err != nil {
t.Fatal(err)
}

// Write data
data := []byte(`{"test": "flush"}`)
if err := writer.Write(data); err != nil {
t.Fatal(err)
}

// Data should be flushed to disk
if err := writer.Close(); err != nil {
t.Fatal(err)
}

// Read file and verify
content, err := os.ReadFile(tmpfile.Name())
if err != nil {
t.Fatal(err)
}

expected := `{"test": "flush"}` + "\n"
if string(content) != expected {
t.Errorf("Expected %q, got %q", expected, string(content))
}
}