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 #66 #115

Merged
merged 6 commits into from
Nov 6, 2024
Merged

fix #66 #115

merged 6 commits into from
Nov 6, 2024

Conversation

KevRiver
Copy link
Contributor

@KevRiver KevRiver commented Nov 2, 2024

  • Write a test that reproduces the problem
  • Fix it. The test should now pass

@aybabtme
Copy link
Collaborator

aybabtme commented Nov 3, 2024

what did you think about the suggestion in #66 to catch scanner.Err() == bufio.ErrTooLong?

@aybabtme aybabtme changed the title fix#66 fix #66 Nov 4, 2024
scanner_test.go Outdated
return time.Date(2024, 11, 1, 15, 40, 0, 0, time.UTC)
}

sink := bufsink.NewSizedBufferedSink(1024*1024, nil)
Copy link
Collaborator

@aybabtme aybabtme Nov 5, 2024

Choose a reason for hiding this comment

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

this sized buffered sink isn't related to the scanning buffer, it's simply buffering [100]*types.LogEvent

Suggested change
sink := bufsink.NewSizedBufferedSink(1024*1024, nil)
sink := bufsink.NewSizedBufferedSink(100, nil)

in fact this test here should inspect the content of that buffer after parsing a message that's too long:

	require.Len(t, sink.Buffered, len(want))
	for i, got := range sink.Buffered {
		require.Equal(t, pjson(want[i]), pjson(got))
	}

i think you want to demonstrate 2 things:

  • the scanner didn't die because of the excessively long input
  • the scanner kept on scanning log events despite the input that was too long

right now the test doesn't demonstrate this, afaict

scanner_test.go Outdated
func TestLargePayload(t *testing.T) {

ctx := context.Background()
payload := `{"msg":` + strings.Repeat("a", 1024*1024) + `}` // more than 1mb long json payload
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
payload := `{"msg":` + strings.Repeat("a", 1024*1024) + `}` // more than 1mb long json payload
payload := `{"msg":` + strings.Repeat("a", 2*1024*1024) + `}` // more than 1mb long json payload

shouldn't this be large enough to overflow the buffer?

also, probably better to use the bufferMaxSize constant

Suggested change
payload := `{"msg":` + strings.Repeat("a", 1024*1024) + `}` // more than 1mb long json payload
payload := `{"msg":` + strings.Repeat("a", bufferMaxSize + 1) + `}` // more than 1mb long json payload

}
break
}
if skipNextScan {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice

@aybabtme aybabtme merged commit ec884b0 into humanlogio:master Nov 6, 2024
6 checks passed
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