You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/09/08 06:14:34 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #7884: fix(file-loger): use no buffering model when open file
monkeyDluffy6017 opened a new pull request, #7884:
URL: https://github.com/apache/apisix/pull/7884
### 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
This bug is introduced in https://github.com/apache/apisix/issues/7839
Fixes #7839
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] 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
- [x] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1324449014
> @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7884:
URL: https://github.com/apache/apisix/pull/7884#discussion_r965557052
##########
apisix/plugins/file-logger.lua:
##########
@@ -116,11 +119,16 @@ local function write_file_data(conf, log_message)
if not file then
core.log.error("failed to open file: ", conf.path, ", error info: ", err)
else
- local ok, err = file:write(msg, '\n')
+ -- file:write(msg, "\n") will call fwrite several times
+ -- which will cause problem with the log output
+ -- it should be atomic
+ msg = msg .. "\n"
+ local ok, err = file:write(msg)
if not ok then
core.log.error("failed to write file: ", conf.path, ", error info: ", err)
else
- file:flush()
+ -- No buffering model don't need flush, write to file directly
+ -- file:flush()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1240465996
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1242963450
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1241397170
> > @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1241411931
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...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1240453566
@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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1324449830
> > @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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #7884:
URL: https://github.com/apache/apisix/pull/7884
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on PR #7884:
URL: https://github.com/apache/apisix/pull/7884#issuecomment-1322979352
@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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] bzp2010 commented on a diff in pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
bzp2010 commented on code in PR #7884:
URL: https://github.com/apache/apisix/pull/7884#discussion_r965557052
##########
apisix/plugins/file-logger.lua:
##########
@@ -116,11 +119,16 @@ local function write_file_data(conf, log_message)
if not file then
core.log.error("failed to open file: ", conf.path, ", error info: ", err)
else
- local ok, err = file:write(msg, '\n')
+ -- file:write(msg, "\n") will call fwrite several times
+ -- which will cause problem with the log output
+ -- it should be atomic
+ msg = msg .. "\n"
+ local ok, err = file:write(msg)
if not ok then
core.log.error("failed to write file: ", conf.path, ", error info: ", err)
else
- file:flush()
+ -- No buffering model don't need flush, write to file directly
+ -- file:flush()
Review Comment:
If they are no longer needed, do we need to delete it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] monkeyDluffy6017 commented on a diff in pull request #7884: fix(file-loger): use no buffering model when open file
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #7884:
URL: https://github.com/apache/apisix/pull/7884#discussion_r965562158
##########
apisix/plugins/file-logger.lua:
##########
@@ -116,11 +119,16 @@ local function write_file_data(conf, log_message)
if not file then
core.log.error("failed to open file: ", conf.path, ", error info: ", err)
else
- local ok, err = file:write(msg, '\n')
+ -- file:write(msg, "\n") will call fwrite several times
+ -- which will cause problem with the log output
+ -- it should be atomic
+ msg = msg .. "\n"
+ local ok, err = file:write(msg)
if not ok then
core.log.error("failed to write file: ", conf.path, ", error info: ", err)
else
- file:flush()
+ -- No buffering model don't need flush, write to file directly
+ -- file:flush()
Review Comment:
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org