Skip to content
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.

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

fix: bufio.Scanner: token too long #19662

Merged
merged 5 commits into from
Sep 29, 2020
Merged

fix: bufio.Scanner: token too long #19662

merged 5 commits into from
Sep 29, 2020

Conversation

stuartcarnie
Copy link
Contributor

@stuartcarnie stuartcarnie commented Sep 29, 2020

Closes #19586

This PR fixes two issues.

  • Fixes a panic that was introduced with a new LineReader type.
  • Adds an option to override the maximum line length in write.Batcher to support line lengths greater than the default 64KiB for a new bufio.Scanner
  • Adds a CLI argument, max-line-length, to configure the maximum line length. The default is 16MB, which should cover the majority of use cases.

This commit adds a field to Batcher to configure the maximum allowed
line length. If this value is non-zero, the bufio.Scanner buffer
will be configured scan lines up to this length. If a line exceeds
this length an ErrLineToLong error will be returned.

Simplified TestBatcher_read tests and added a test case for this
new scenario and to ensure lines exceeding the default
bufio.MaxScanTokenSize are allowed.
@stuartcarnie stuartcarnie added kind/bug area/2.x OSS 2.0 related issues and PRs labels Sep 29, 2020
Copy link
Contributor

@rogpeppe rogpeppe left a comment

Choose a reason for hiding this comment

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

I'm afraid I didn't have time to finish looking through the code before eod, but here's one small comment in the meantime... (looks good otherwise as far as I saw so far!)

write/batcher.go Outdated
}

// limit the initial size of the buffer to a maximum of bufio.MaxScanTokenSize bytes
scanner.Buffer(make([]byte, min(bufio.MaxScanTokenSize, maxLineLength)), maxLineLength)
Copy link
Contributor

Choose a reason for hiding this comment

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

Or just scanner.Buffer(nil, maxLineLength) and let the buffer be grown as needed when lines are read. That way when all lines are short (presumably a common case) we'll use a lot less memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good feedback – consider it done!

Copy link
Contributor

@brettbuddin brettbuddin left a comment

Choose a reason for hiding this comment

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

Nice find!

@stuartcarnie stuartcarnie merged commit b3b8f9e into master Sep 29, 2020
@stuartcarnie stuartcarnie deleted the sgc/issue/19660 branch September 29, 2020 20:44
Copy link
Contributor

@e-dard e-dard left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/2.x OSS 2.0 related issues and PRs kind/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cli write: Error: Failed to write data: bufio.Scanner: token too long
4 participants