Skip to content

fix(file-loger): use no buffering model when open file#7884

Merged
spacewander merged 3 commits into
apache:masterfrom
juststillthinking:fix-file-logger-output-mixed-bug
Nov 23, 2022
Merged

fix(file-loger): use no buffering model when open file#7884
spacewander merged 3 commits into
apache:masterfrom
juststillthinking:fix-file-logger-output-mixed-bug

Conversation

@juststillthinking

@juststillthinking juststillthinking commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

Description

use no buffering model when open file
Avoid error:
If the data written to the log file is larger than the default buffer, it will be flushed into the file several times, and other log may be flushed in during this period

Fixes #7839

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Avoid error:
If the data written to the log file is larger than the default buffer, it will be flushed into the file several times, and other log may be flushed in during this period
This bug is introduced in apache#7839
Comment thread apisix/plugins/file-logger.lua Outdated
Comment on lines +130 to +131
-- No buffering model don't need flush, write to file directly
-- file:flush()

@bzp2010 bzp2010 Sep 8, 2022

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.

If they are no longer needed, do we need to delete it? BTW, the code lint point this too, there is an empty if branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@tokers

tokers commented Sep 8, 2022

Copy link
Copy Markdown
Contributor

@monkeyDluffy6017 I don't think just changing the flush mechanism is a good idea, but it can be an alternative for the users. Also, the default Glibc buffer size should be shown (or the way we can calculate it should be given if the buffer size varies on different OS).

@juststillthinking

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 I don't think just changing the flush mechanism is a good idea, but it can be an alternative for the users. Also, the default Glibc buffer size should be shown (or the way we can calculate it should be given if the buffer size varies on different OS).

I don't think we need buffer here, because we write and flush immediately. cc @spacewander

@tokers

tokers commented Sep 9, 2022

Copy link
Copy Markdown
Contributor

@monkeyDluffy6017 I don't think just changing the flush mechanism is a good idea, but it can be an alternative for the users. Also, the default Glibc buffer size should be shown (or the way we can calculate it should be given if the buffer size varies on different OS).

I don't think we need buffer here, because we write and flush immediately. cc @spacewander

The number of access logs can be tremendous if the QPS are high, in such a case, the performance will suffer from a serious degradation. Access logs are different from error logs, we definitely need to consider the caching. No buffer can be an option but MUST NOT BE a required one.

@spacewander

Copy link
Copy Markdown
Member

What about using a bigger buffer, like 64KB in Nginx's access_log?

IMHO, there still will have an issue that a log > 64KB might be broken. Maybe using C.write and managing the buffer by ourselves via Lua table will be a better idea...

@spacewander

Copy link
Copy Markdown
Member

After rethinking this problem, I have a new opinion:
As currently we create the file handler per route instead of per file, it is hard to flush the log in order if we use a buffer per route.

Since the original code doesn't use buffer (it flushes the buffer immediately), I think we can focus on solving the bug first.

@juststillthinking

Copy link
Copy Markdown
Contributor Author

@tokers do you agree with @spacewander ?

Since the original code doesn't use buffer (it flushes the buffer immediately), I think we can focus on solving the bug first.

@tokers

tokers commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

@tokers do you agree with @spacewander ?

Since the original code doesn't use buffer (it flushes the buffer immediately), I think we can focus on solving the bug first.

Got it.

@tokers

tokers commented Nov 23, 2022

Copy link
Copy Markdown
Contributor

@tokers do you agree with @spacewander ?

Since the original code doesn't use buffer (it flushes the buffer immediately), I think we can focus on solving the bug first.

Got it.

But we may still open an eye about the performance.

@spacewander
spacewander merged commit 6c6b077 into apache:master Nov 23, 2022
spacewander pushed a commit to spacewander/incubator-apisix that referenced this pull request Jan 28, 2023
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.

bug: [file-logger] two log messages mix-output in one line(两条不同的日志混合在一行输出)

5 participants