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

buffer: fix emit error of race condition #4447

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

daipom
Copy link
Contributor

@daipom daipom commented Mar 26, 2024

Which issue(s) this PR fixes:

What this PR does / why we need it:
After 95438b2 (#4342) (v1.16.4), there is a section where chunks do not have a lock in write_step_by_step().

write_step_by_step() must ensure their locks until passing them to the block.
Otherwise, race condition can occur and it can cause emit error by IOError.
Example of warning messages of emit error:

[warn]: #0 emit transaction failed: error_class=IOError error="closed stream" location=...
[warn]: #0 send an error event stream to @ERROR: error_class=IOError error="closed stream" location=...

Docs Changes:
Not needed.

Release Note:
Buffer: Fix emit error of v1.16.4 sometimes failing to process large data exceeding chunk size limit

@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

It looks like a lot of diffs, but most of them are just indentation changes.

I have changed chunk.synchronize to chunk.mon_enter.
It causes an indentation change of the following block.

@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

There are three changes in this PR:

  • Changed chunk.synchronize to chunk.mon_enter.
  • Added chunk.mon_exit to the rescue block for ShouldRetry.
  • Added the ensure block and ensured to execute chunk.mon_exit.

@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

I am considering if I can add some tests...

@kenhys
Copy link
Contributor

kenhys commented Mar 26, 2024

Even though it will be squashed, split indentation commit may be better.

@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

Even though it will be squashed, split indentation commit may be better.

I see!!

After 95438b2 (fluent#4342), there is a
section where chunks do not have a lock in `write_step_by_step()`.

`write_step_by_step()` must ensure their locks until passing them
to the block.
Otherwise, race condition can occur and it can cause emit error
by IOError.
Example of warning messages of emit error:

    [warn]: #0 emit transaction failed: error_class=IOError error="closed stream" location=...
    [warn]: #0 send an error event stream to @error: error_class=IOError error="closed stream" location=...

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
I separated commits for clarity.

Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom daipom force-pushed the buffer-fix-emit-error-of-race-condition branch from 61a2b5a to fedb6d4 Compare March 26, 2024 09:10
@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

Even though it will be squashed, split indentation commit may be better.

I see!!

Thanks! I have separated them.
The actual fixes are f4a4591

@daipom daipom marked this pull request as ready for review March 26, 2024 09:35
@daipom
Copy link
Contributor Author

daipom commented Mar 26, 2024

I will check the behavior a little more tomorrow and add some tests if possible, but basically, I think this is prepared.

@daipom daipom changed the title Draft: buffer: fix emit error of race condition buffer: fix emit error of race condition Mar 27, 2024
Signed-off-by: Daijiro Fukuda <fukuda@clear-code.com>
@daipom
Copy link
Contributor Author

daipom commented Mar 27, 2024

I have added a test.

@daipom daipom requested a review from ashie March 27, 2024 02:37
@daipom daipom added this to the v1.16.5 milestone Mar 27, 2024
Copy link
Member

@ashie ashie left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks!

@daipom
Copy link
Contributor Author

daipom commented Mar 27, 2024

Thanks!

@daipom daipom merged commit 13a5199 into fluent:master Mar 27, 2024
16 checks passed
@daipom daipom deleted the buffer-fix-emit-error-of-race-condition branch March 27, 2024 04:17
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.

Buffer: v1.16.4: Emit error by IOError
3 participants