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

ensure inputSize state value is reset during buftok.flush #16760

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

jsvd
Copy link
Member

@jsvd jsvd commented Dec 5, 2024

The BufferedTokenizer.flush must reset the inputSize value back to zero on flush, otherwise it will grow until a buffer full exception is thrown if the buffer has a size limit.

We can see the test fails if the inputSize = 0 is not present:

❯ bin/rspec logstash-core/spec/logstash/util/buftok_spec.rb -fd -e flush
[...]
FileWatch::BufferedTokenizer
  flush
    emits the contents of the buffer
    resets the state of the buffer
    with decode_size_limit_bytes
      resets the state of the buffer (FAILED - 1)
      emits the contents of the buffer

Failures:

  1) FileWatch::BufferedTokenizer flush with decode_size_limit_bytes resets the state of the buffer
     Failure/Error: expect(subject).to be_empty
       expected `#<FileWatch::BufferedTokenizer:0x35977ba7>.empty?` to be truthy, got false
     # ./logstash-core/spec/logstash/util/buftok_spec.rb:65:in `block in <main>'
     # ./spec/spec_helper.rb:84:in `block in <main>'

But with it tests pass:

❯ bin/rspec logstash-core/spec/logstash/util/buftok_spec.rb -fd -e flush

FileWatch::BufferedTokenizer
  flush
    resets the state of the buffer
    emits the contents of the buffer
    with decode_size_limit_bytes
      emits the contents of the buffer
      resets the state of the buffer

Finished in 0.0246 seconds (files took 1.34 seconds to load)
4 examples, 0 failures

closes #16762

Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

@@ -106,12 +106,13 @@ public RubyArray extract(final ThreadContext context, IRubyObject data) {
public IRubyObject flush(final ThreadContext context) {
final IRubyObject buffer = input.join(context);
input.clear();
inputSize = 0;
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼 this change does actually fix the issue where inputSize is not reset on a flush.

Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM to fix.

@yaauie yaauie merged commit e36cace into elastic:main Dec 9, 2024
6 checks passed
@yaauie
Copy link
Member

yaauie commented Dec 9, 2024

@logstashmachine backport 8.x 8.17 8.16

[EDIT: the bot doesn't like multiple branches in a single comment]

@yaauie
Copy link
Member

yaauie commented Dec 9, 2024

@logstashmachine backport 8.x

@yaauie
Copy link
Member

yaauie commented Dec 9, 2024

@logstashmachine backport 8.17

@yaauie
Copy link
Member

yaauie commented Dec 9, 2024

@logstashmachine backport 8.16

github-actions bot pushed a commit that referenced this pull request Dec 9, 2024
github-actions bot pushed a commit that referenced this pull request Dec 9, 2024
donoghuc pushed a commit that referenced this pull request Dec 9, 2024
…16769)

(cherry picked from commit e36cace)

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
donoghuc pushed a commit that referenced this pull request Dec 9, 2024
…16770)

(cherry picked from commit e36cace)

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
yaauie pushed a commit that referenced this pull request Dec 9, 2024
…16771)

(cherry picked from commit e36cace)

Co-authored-by: João Duarte <jsvd@users.noreply.github.com>
@jsvd jsvd deleted the fix_buftok_flush_state_reset branch December 10, 2024 09:49
@jsvd jsvd added v8.17.1 and removed v8.17.0 labels Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BufferedTokenizerExt does not clear size marker on flush
3 participants