Skip to content

Fix LineBuffer payload byte accounting - #98

Open
Sagar07-star wants to merge 2 commits into
Netflix:mainfrom
Sagar07-star:fix/line-buffer-byte-size
Open

Sagar07-star wants to merge 2 commits into
Netflix:mainfrom
Sagar07-star:fix/line-buffer-byte-size

Conversation

@Sagar07-star

@Sagar07-star Sagar07-star commented Oct 10, 2026 •

Copy link
Copy Markdown

Summary
Count the UTF-8 payload bytes, including newline separators, when deciding whether LineBuffer should flush.
Problem
LineBuffer currently counts only characters in each input line, while SocketWriter sends the newline-joined buffer encoded as UTF-8. For example, two three-character lines produce seven payload bytes but count as six, so a seven-byte threshold does not trigger a flush.
Changes

  • Include one separator byte for each line after the first.
  • Count encoded UTF-8 bytes instead of characters.
  • Add regression coverage for separator-triggered flushing, multibyte text, empty lines, and size reset after flushing.
    The threshold still triggers after appending a whole line; this does not introduce a strict datagram size cap.
    Validation
  • All four LineBuffer tests fail before the fix and pass afterward (including the corrected existing size expectation).
  • Python 3.12: python -m unittest discover -s tests -t . -v: 121 passed, two Unix-domain-socket tests blocked by PermissionError in the execution environment.
  • git diff --check passes.

@brharrington brharrington left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The accounting is correct: counting UTF-8 bytes plus one separator per line after the first matches what SocketWriter sends, so buffer_size now means payload bytes. Two notes:

  1. Interaction with #95: append now encodes the line, so a lone surrogate like "\ud800" raises UnicodeEncodeError inside append rather than at flush time. #95's test_udp_close_closes_socket_on_encode_error writes "\ud800" and expects close() to raise, so write() would raise first. Whichever PR lands second needs that test reworked. The new behavior is arguably better (a bad line is rejected before it is stored instead of breaking the flush for neighboring lines), but the exception still propagates out of write() since _write_buffer only catches IOError.
  2. This adds an encode on every write. len(line) if line.isascii() else len(line.encode("utf-8")) avoids the allocation for the common ASCII case.

Avoid encoding ASCII lines, verify invalid UTF-8 preserves buffered data,
and adapt the close error regression test after PR Netflix#95.
@Sagar07-star

Copy link
Copy Markdown
Author

Addressed both review points and updated the tests for #95. Could a maintainer approve the workflow run?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants