feat: use bufio scanner to decode events#45
Conversation
|
@hazzik Could you rebase and resolve the conflicts? (I'm no maintainer 😅) |
|
@reneleonhardt done |
| s := bufio.NewScanner(r) | ||
| for s.Scan() { | ||
| line := s.Bytes() | ||
|
|
There was a problem hiding this comment.
The buffer size of bufio.Scanner is 64 kB, which might not be enough for some inputs. If anything, I'd suggest using bufio.Reader and its ReadString or ReadBytes methods.
There was a problem hiding this comment.
Pull request overview
This PR refactors the SSE event decoder to use bufio.Scanner instead of io.ReadAll for a more memory-efficient streaming approach. Rather than loading the entire input into memory before processing, the decoder now processes events line-by-line as they're read.
Key changes:
- Replaced
io.ReadAllwithbufio.Scannerfor streaming line-by-line processing - Improved memory efficiency for large SSE streams by eliminating the need to buffer all input
- Enhanced error handling by returning scanner errors through
s.Err()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // or a single U+000D CARRIAGE RETURN (CR) character. | ||
| lines := bytes.Split(buf, []byte{'\n'}) | ||
| for _, line := range lines { | ||
| s := bufio.NewScanner(r) |
There was a problem hiding this comment.
bufio.Scanner has a default maximum token size of 64KB. If SSE events can contain lines longer than this limit, the scanner will return an error (bufio.ErrTooLong).
If large events are expected, consider using s.Buffer() to increase the maximum token size:
s := bufio.NewScanner(r)
// Optional: increase buffer size if needed
// s.Buffer(make([]byte, 0, 64*1024), maxTokenSize)Alternatively, if this size limit is acceptable for your use case, consider adding validation or documentation about this constraint.
| s := bufio.NewScanner(r) | |
| s := bufio.NewScanner(r) | |
| // Increase buffer size to support large SSE events (default is 64KB) | |
| const maxTokenSize = 1024 * 1024 // 1MB | |
| s.Buffer(make([]byte, 0, 64*1024), maxTokenSize) |
| // TODO (and unit tests) | ||
| // Lines must be separated by either a U+000D CARRIAGE RETURN U+000A LINE FEED (CRLF) character pair, | ||
| // a single U+000A LINE FEED (LF) character, | ||
| // or a single U+000D CARRIAGE RETURN (CR) character. |
There was a problem hiding this comment.
The TODO comment on line 44 mentions that SSE lines can be separated by CRLF, LF, or CR, but there are no tests verifying CRLF handling. Since bufio.Scanner handles CRLF differently than the previous bytes.Split approach (it strips the \r), consider adding test cases with CRLF line endings to verify the decoder works correctly with Windows-style line endings.
Example test case:
events, err := Decode(bytes.NewBufferString("data: test\r\nevent: message\r\n\r\n"))
No description provided.