[fix] - Propagate Async File Handling Errors #3403

ahrav · 2024-10-13T03:00:03Z

Description:

This PR updates error reporting during file processing. Previously, each Handler spawned a goroutine to handle file processing and returned a channel for the caller to collect results. However, this method only returned processed data and failed to propagate errors due to the file processing happening in separate goroutines. As a result, errors were simply logged, and critical errors were returned within the goroutine, leaving the calling function unaware of any issues. This became especially problematic when HandleFile was called via handleBinary, as the reader passed to HandleFile is a pipe. If the caller isn’t informed of an error, it can't properly consume the reader, potentially leaving it open and exhausting resources.

To resolve this, the PR introduces the DataOrErr struct, allowing handlers to return both critical and non-critical errors to the caller. This ensures better resource management and prevents resource leaks.

Checklist:

Tests passing (make test-community)?
Lint passing (make lint this requires golangci-lint)?

ahrav · 2024-10-13T03:01:51Z

pkg/iobuf/bufferedreaderseeker.go

 // It takes an io.Reader and checks if it supports seeking.
 // If the reader supports seeking, it is stored in the seeker field.
-func NewBufferedReaderSeeker(r io.Reader) *BufferedReadSeeker {


ahrav · 2024-10-13T03:05:22Z

pkg/iobuf/bufferedreaderseeker.go

@@ -82,9 +82,6 @@ func NewBufferedReaderSeeker(r io.Reader) *BufferedReadSeeker {
 	)

 	seeker = asSeeker(r)
-	if seeker == nil {


This was removed to avoid checking out a buffer eagerly. We already lazily check out a buffer when needed.

rgmz · 2024-10-13T03:38:53Z

🐇 🕳️

rosecodym · 2024-10-15T19:18:10Z

pkg/handlers/ar.go

 	}

 	go func() {
-		ctx, cancel := logContext.WithTimeout(ctx, maxTimeout)


Why did you remove this?

I thought I left a comment, but it was probably on the closed PR. This was removed because the context timeout is set at the call site, so setting it here has no effect since it inherits the context from HandleFile.
here

rosecodym · 2024-10-15T19:20:44Z

pkg/handlers/archive.go

-					err = fmt.Errorf("panic occurred: %v", r)
+					panicErr = fmt.Errorf("panic occurred: %v", r)
+				}
+				ctx.Logger().Error(panicErr, "Panic occurred when attempting to open archive")


Why is this error in particular loggable?

It shouldn't be. Logging should be handled while consuming from the dataOrErrChan at the call-site. Will remove thanks.

rosecodym · 2024-10-15T19:23:10Z

pkg/handlers/handlers.go

+var (
+	ErrEmptyReader = errors.New("reader is empty")
+
+	// ErrCriticalProcessing indicates a critical error that should halt processing.


"Critical" is one of those words that unfortunately doesn't really convey any information to someone who doesn't already know the domain. Should these errors halt processing of the source? The file? Something else?

Yea, I agree. I'll update it. 👍

rosecodym · 2024-10-15T19:25:01Z

pkg/handlers/handlers.go

+
+	// If an error occurs during MIME type detection, it is important we close the BufferedReaderSeeker
+	// to release any resources it holds (checked out buffers or temp file).
+	var err error


err gets shadowed later in this function. If that's on purpose, I find it very confusing, and would understand much better if you used a different variable name.

rosecodym · 2024-10-15T19:26:34Z

pkg/handlers/handlers.go

+// handler to manage file extraction or processing.
+//
+// The function will return nil (success) in the following cases:
+// - If the reader is empty (ErrEmptyReader)


Will it return nil? Or ErrEmptyReader?

It should return nil if the error from newFileReader is ErrEmptyReader. Do think think it should return ErrEmptyReader instead?

No, that makes sense, I was just confused because within the context the comment it wasn't clear that an empty reader was signaled (elsewhere) by ErrEmptyReader

rosecodym · 2024-10-15T19:53:44Z

pkg/handlers/rpm.go

 	}

 	go func() {
-		ctx, cancel := logContext.WithTimeout(ctx, maxTimeout)


same question as above: why the removal?

rosecodym · 2024-10-15T19:56:55Z

pkg/handlers/rpm.go

 			}
 		}()

 		var rpm *rpmutils.Rpm
 		rpm, err = rpmutils.ReadRpm(input)
 		if err != nil {
-			ctx.Logger().Error(err, "error reading RPM")


Are these not real errors?

I understand why you thought I removed them. 😢 In the defer func(), we call .measureLatencyAndHandleErrors, which uses the shadowed err. This eventually gets logged when we consume from dataOrErrChan. I can avoid shadowing by explicitly sending err to the channel.

rosecodym

I see what looks like three separable PRs here:

Moving around some metric observations
Switching to DataOrErr
Surfacing a new archive extraction error

Could they be done separately?

rosecodym · 2024-10-22T19:00:30Z

pkg/handlers/handlers.go

+// handler to manage file extraction or processing.
+//
+// The function will return nil (success) in the following cases:
+// - If the reader is empty (ErrEmptyReader)


No, that makes sense, I was just confused because within the context the comment it wasn't clear that an empty reader was signaled (elsewhere) by ErrEmptyReader

ahrav · 2024-10-22T19:06:39Z

I see what looks like three separable PRs here:

Moving around some metric observations

Switching to DataOrErr

Surfacing a new archive extraction error

Could they be done separately?

Yep yep, will do. 👍

mcastorina · 2024-10-23T06:53:08Z

pkg/handlers/handlers.go

+// DataOrErr represents a result that can either contain data or an error.
+// The Data field holds the byte slice of data, and the Err field holds any error that occurred.
+// This structure is used to handle asynchronous file processing where each chunk of data
+// or potential error needs to be communicated back to the caller. It allows for
+// efficient streaming of file contents while also providing a way to propagate errors
+// that may occur during the file handling process.
+type DataOrErr struct {
+	Data []byte
+	Err  error
+}


Just noticing that DataOrErr pretty much describes Rust's Result enum. There are some useful methods if you wanted to follow their pattern.

🦀 🦀 🦀

Yeah, if we do go down this route in a more organized way it'd be good to look at prior art. Rust's version of this is probably the most most usable by people other than functional programming weirdos. Kotlin has a third-party version that references the concept's monadic roots and references some prior art like the F# version and the inscrutable Haskell dorks.

ahrav added 4 commits October 12, 2024 09:31

fix resource leak

da46890

add comment

a505f07

use errors.Join

44d9c43

report errors during file processing.

264298f

ahrav force-pushed the bug-fix-potential-resource-leaks branch from 8268de0 to 264298f Compare October 13, 2024 03:00

ahrav commented Oct 13, 2024

View reviewed changes

ahrav added 3 commits October 12, 2024 20:08

revert

abe12fd

adjust comment

a07ffbe

remove unused

44f080a

add more tests

62561fb

ahrav marked this pull request as ready for review October 14, 2024 02:19

ahrav requested review from a team as code owners October 14, 2024 02:19

Base automatically changed from fix-file-descriptor-exhaustion to main October 15, 2024 19:11

rosecodym reviewed Oct 15, 2024

View reviewed changes

ahrav mentioned this pull request Oct 18, 2024

[Fix] - buffer leak #3472

Closed

2 tasks

ahrav added 3 commits October 19, 2024 19:41

address comments

9411abd

merge main

ca82c72

update verbiage of comment

201883a

ahrav requested review from a team and rosecodym October 20, 2024 02:49

rosecodym reviewed Oct 22, 2024

View reviewed changes

mcastorina reviewed Oct 23, 2024

View reviewed changes

This was referenced Oct 28, 2024

[refactor] - Adjust File Handling Errors #3519

Merged

[refactor] - Add DataOrErr #3520

Merged

[feat] - Introduce Fatal/Non-Fatal File Handling Errors #3521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] - Propagate Async File Handling Errors #3403

[fix] - Propagate Async File Handling Errors #3403

ahrav commented Oct 13, 2024

ahrav Oct 13, 2024

ahrav Oct 13, 2024

rgmz commented Oct 13, 2024

rosecodym Oct 15, 2024

ahrav Oct 18, 2024

rosecodym Oct 15, 2024

ahrav Oct 18, 2024

rosecodym Oct 15, 2024

ahrav Oct 18, 2024

rosecodym Oct 15, 2024

rosecodym Oct 15, 2024

ahrav Oct 18, 2024

rosecodym Oct 22, 2024

rosecodym Oct 15, 2024

rosecodym Oct 15, 2024

ahrav Oct 18, 2024

rosecodym left a comment

rosecodym Oct 22, 2024

ahrav commented Oct 22, 2024

mcastorina Oct 23, 2024

ahrav Oct 26, 2024

rosecodym Oct 29, 2024 •

edited

Loading

[fix] - Propagate Async File Handling Errors #3403

Are you sure you want to change the base?

[fix] - Propagate Async File Handling Errors #3403

Conversation

ahrav commented Oct 13, 2024

Description:

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgmz commented Oct 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosecodym left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahrav commented Oct 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosecodym Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

rosecodym Oct 29, 2024 •

edited

Loading