Consolidate the Reader and Writer side #2
Conversation
bep
force-pushed
the
feat/consolidate
branch
from
a40d308 to
60080fd
Compare
There was a problem hiding this comment.
Pull request overview
This PR consolidates the Reader and Writer implementations by refactoring the Reader to internally use the Writer component. The previous implementation used a buffered reader approach with marker detection, while the new implementation uses a pipe-based architecture where data flows through a Writer that demultiplexes text and binary streams into separate pipes, which are then processed by goroutines.
Key changes:
- Refactored
NewReaderto returnio.ReadCloser(previouslyio.Reader) and use a goroutine-based architecture with pipes - Renamed the constructor from
NewtoNewReaderfor consistency - Added mutex protection in tests to handle concurrent access to shared slices from blob handlers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| reader.go | Complete architectural rewrite using Writer internally with goroutines and pipes instead of buffered marker detection |
| reader_test.go | Updated to use NewReader constructor, added mutex protection for concurrent blob handling, added Close() calls, and added new benchmark |
| writer_test.go | Simplified test setup by using standard io.Pipe() instead of custom pipeReadWriteCloser type, added new benchmark |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bep
force-pushed
the
feat/consolidate
branch
2 times, most recently
from
87f0d75 to
16f28c3
Compare
bep
force-pushed
the
feat/consolidate
branch
from
16f28c3 to
176e4f5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for b.Loop() { | ||
| r := New(&buf, bh) | ||
| r := NewReader(&buf, bh) | ||
| _, err := io.Copy(io.Discard, r) | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| err = r.Close() | ||
| if err != nil { | ||
| b.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The benchmark reuses the same buffer in each iteration without resetting it. After the first iteration, the buffer is fully consumed (EOF), so subsequent iterations will immediately return EOF without actually benchmarking the reader logic. The buffer should be reset or recreated for each iteration to ensure accurate benchmarking.
| // The reader passed to it receives the blob. Any non-consumed data will be discarded. | ||
| handleBlob func(id uint32, r io.Reader) error | ||
| } | ||
| g, _ := errgroup.WithContext(context.Background()) |
There was a problem hiding this comment.
The errgroup is created with context.Background() and the returned context is discarded. This means there's no way to cancel the goroutines if needed. If the caller abandons reading without calling Close(), the goroutines will continue running. Consider using the context or documenting that Close() must be called to properly clean up resources.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.