You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by danobi <gi...@git.apache.org> on 2016/04/13 22:09:45 UTC
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
GitHub user danobi opened a pull request:
https://github.com/apache/trafficserver/pull/568
TS-4072 Diagnostic log rolling races
This patch prevents race conditions when a diagnostic log rolls. The patch is a little longer than I had hoped for because of error handling conditions.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/danobi/trafficserver TS-4072
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/568.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #568
----
commit ed30d422efd71333722f4124d5eebeae037a9750
Author: Daniel Xu <dl...@yahoo.com>
Date: 2016-04-13T19:34:34Z
TS-4072 Diagnostic log rolling races
Use preexisting mutex to prevent race condition when rotating
diags_log, stdout_log, and stderr_log.
commit 797553e67140349d8f49b7f0840e397087fd8047
Author: Daniel Xu <dl...@yahoo.com>
Date: 2016-04-13T19:35:09Z
Improve diagnostic logs logging statements
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219324620
@zwoop I've gone ahead and done that. I don't know how (or if I can) to run the CI builds so I'll just leave that to you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63826235
--- Diff: lib/ts/Diags.cc ---
@@ -625,11 +625,14 @@ Diags::should_roll_diagslog()
fflush(diags_log->m_fp);
if (diags_log->roll()) {
--- End diff --
agree with u, old file will be closed by ~~LINE 635: delete old_diags;~~.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632436
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
return false;
- if (stdout_log) {
- delete stdout_log;
- stdout_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stdout_log = new BaseLogFile(_bind_stdout);
+ BaseLogFile *old_stdout_log = stdout_log;
+ BaseLogFile *new_stdout_log = new BaseLogFile(_bind_stdout);
// on any errors we quit
- if (!stdout_log || stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
+ if (!new_stdout_log || new_stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
fprintf(stderr, "[Warning]: unable to open file=%s to bind stdout to\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
--- End diff --
This would be better as ``log_log_error()``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209628730
Looks good to me. Dan, Jason, and I discussed this at length prior to the work and this looks in line with our concensus.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/568
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/371/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220702623
Fixed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218525646
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the issue:
https://github.com/apache/trafficserver/pull/568
@zwoop clang-formatted & rebased
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218614205
Linux (CentOS7) build finished successfully. Details on https://ci.trafficserver.apache.org/job/Github-Linux/17/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632366
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
- /*
log_log_trace("should_roll_outputlog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", outputlog_rolling_enabled,
- outputlog_rolling_size, outputlog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - outputlog_time_last_roll);
- log_log_trace("stdout_log = %p\n", stdout_log);
- */
+ log_log_trace("should_roll_outputlog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
--- End diff --
I think ``log_log_trace`` ought to be something you can set at runtime. Probably an environment variable or something that sets a global flag?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59629895
--- Diff: lib/ts/Diags.cc ---
@@ -642,10 +642,10 @@ Diags::should_roll_diagslog()
bool ret_val = false;
log_log_trace("should_roll_diagslog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", diagslog_rolling_enabled,
- diagslog_rolling_size, diagslog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - diagslog_time_last_roll);
+ log_log_trace("should_roll_diagslog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
+ diagslog_rolling_enabled, diagslog_rolling_size, diagslog_rolling_interval);
+ log_log_trace("should_roll_diagslog(): RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
+ log_log_trace("should_roll_diagslog(): time(0) - last_roll_time = %d\n", time(0) - diagslog_time_last_roll);
--- End diff --
Use __func__ to generate the function name.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218904299
Linux (CentOS7) build failed! Details on https://ci.trafficserver.apache.org/job/Github-Linux/34/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220376323
.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209663459
OK the locking makes sense to me I think. You have to hold the lock because you want to serialize WRT printing output.
``Diags::rebind_stderr`` and ``Diags::rebind_stdout`` don't need to be part of the class interface. You can replace both of these with a single local static helper:
```C
bool rebind(int oldfd, int newfd) {
/* ... error handling ... */
return dup2(newfd, oldfd) == 0;
}
```
Please write unit tests for this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59890598
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
- /*
log_log_trace("should_roll_outputlog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", outputlog_rolling_enabled,
- outputlog_rolling_size, outputlog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - outputlog_time_last_roll);
- log_log_trace("stdout_log = %p\n", stdout_log);
- */
+ log_log_trace("should_roll_outputlog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
--- End diff --
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210501721
I don't see any reason to block writing tests on some proposed framework. All you really need to write a test is a hook to run it (which we have) and a way to check state (which we have). I think that is is possible to write an actual unit test for this, focusing on just the ``Diags`` class, but for now I'd also be ok with a shell-script or something like what we used for testing access log rolling in the first iteration of TSQA.
Off the top of my head, how about a test that starts a few threads writing diagnostics, then a second set of threads that continually rebinds ``stderr`` and ``stdout`` to ``/dev/null``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218782605
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209691924
@jpeach Is there a preferred framework for unit testing? (ie how/where do I put the tests?)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59632028
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
return false;
- if (stdout_log) {
- delete stdout_log;
- stdout_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stdout_log = new BaseLogFile(_bind_stdout);
+ BaseLogFile *old_stdout_log = stdout_log;
+ BaseLogFile *new_stdout_log = new BaseLogFile(_bind_stdout);
--- End diff --
Can we find a better argument name than ``_bind_stdout``? Maybe ``path`` or ``stdout_path``?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59889285
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
- /*
log_log_trace("should_roll_outputlog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", outputlog_rolling_enabled,
- outputlog_rolling_size, outputlog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - outputlog_time_last_roll);
- log_log_trace("stdout_log = %p\n", stdout_log);
- */
+ log_log_trace("should_roll_outputlog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
--- End diff --
Yes; the compilation time option means that is it subject to bitrot and that few people will be able ti use it. I'm OK with addressing this as a separate change, but I think it is worth addressing.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219261334
This needs a rebase and new clang-format indentation. It fails with
https://ci.trafficserver.apache.org/view/github/job/Github-Linux/67/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219186505
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220231207
@chaiman I see what you mean. I'll fix that in a separate ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/568
[approve ci] @PSUdaemon @jpeach thoughts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219190153
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218929112
Upgraded git on all the build boxes, so try again [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219347020
@zwoop done
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/568
Well, this slipped through the net. @danobi Can you please make an update to the PR, that does not have merge conflicts?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218905112
FreeBSD v10 build finished successfully. Details on https://ci.trafficserver.apache.org/job/Github-FreeBSD/17/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219194926
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #568: TS-4072 Diagnostic log rolling races
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafficserver/pull/568
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-209664725
Yes. We looked at various approaches and decided on this one because there was already a lock around access to the file descriptor. Locking the update to that value should prevent the race condition and potential use of a deleted file handle.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59644646
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
- /*
log_log_trace("should_roll_outputlog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", outputlog_rolling_enabled,
- outputlog_rolling_size, outputlog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - outputlog_time_last_roll);
- log_log_trace("stdout_log = %p\n", stdout_log);
- */
+ log_log_trace("should_roll_outputlog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
--- End diff --
Yeah I was thinking about that when I first wrote the function. I'm not sure what the best way to do it would be. Right now it's compile time defined with a `#define`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59631832
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
return false;
- if (stdout_log) {
- delete stdout_log;
- stdout_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stdout_log = new BaseLogFile(_bind_stdout);
+ BaseLogFile *old_stdout_log = stdout_log;
+ BaseLogFile *new_stdout_log = new BaseLogFile(_bind_stdout);
// on any errors we quit
- if (!stdout_log || stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
+ if (!new_stdout_log || new_stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
fprintf(stderr, "[Warning]: unable to open file=%s to bind stdout to\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- if (!stdout_log->m_fp) {
+ if (!new_stdout_log->m_fp) {
fprintf(stderr, "[Warning]: file pointer for stdout %s = NULL\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- return rebind_stdout(fileno(stdout_log->m_fp));
+ // now exchange the stdout_log pointer
+ lock();
+ stdout_log = new_stdout_log;
+ bool ret = rebind_stdout(fileno(new_stdout_log->m_fp));
--- End diff --
If this fails, are we in a well-defined state? No caller ever checks the return value of this ... what would happen if it fails?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by chaiman <gi...@git.apache.org>.
Github user chaiman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63659601
--- Diff: lib/ts/Diags.cc ---
@@ -625,11 +625,14 @@ Diags::should_roll_diagslog()
fflush(diags_log->m_fp);
if (diags_log->roll()) {
--- End diff --
The old file don't need to be closed in diags_log->roll(). close old file should at destructor of old_diags. in addition,
this will cause crash because of writing to closed file
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210516157
@jpeach Thanks for the comments.
I've addressed the relevant issues you brought up in the force pushed commits. I've also gone ahead and made a separate ticket with the rest of the issues [Jira ticket](https://issues.apache.org/jira/browse/TS-4354).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-210488274
While I think unit tests are in general a good idea, I'm not sure how that could be done in this case and not in the time frame for committing this to 6.2. I expect tests will need to wait for a better test framework (via TSQA or successor) because of the interaction with the file system. This is especially acute due to the lack of any pre-existing unit tests which could be extended.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218612393
[approve ci]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219342137
Can you squash the commits together too? We don't want intermediary commits that are failing (they make git bisect difficult to deal with).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59633334
--- Diff: lib/ts/Diags.cc ---
@@ -716,14 +722,12 @@ Diags::should_roll_outputlog()
bool ret_val = false;
bool need_consider_stderr = true;
- /*
log_log_trace("should_roll_outputlog() was called\n");
- log_log_trace("rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n", outputlog_rolling_enabled,
- outputlog_rolling_size, outputlog_rolling_interval);
- log_log_trace("RollingEnabledValues::ROLL_ON_TIME = %d\n", RollingEnabledValues::ROLL_ON_TIME);
- log_log_trace("time(0) - last_roll_time = %d\n", time(0) - outputlog_time_last_roll);
- log_log_trace("stdout_log = %p\n", stdout_log);
- */
+ log_log_trace("should_roll_outputlog(): rolling_enabled = %d, output_rolling_size = %d, output_rolling_interval = %d\n",
--- End diff --
That's outside the scope of this fix. It should be in a separate patch on a different ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by chaiman <gi...@git.apache.org>.
Github user chaiman commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r63660164
--- Diff: lib/ts/Diags.cc ---
@@ -642,11 +645,14 @@ Diags::should_roll_diagslog()
if (diags_log->roll()) {
--- End diff --
same problem
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-218903748
One more round of builds [approve ci].
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220791012
@PSUdaemon There's not any issues I'm aware of at the moment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-220764504
Are there still outstanding issues here or is it possible to merge this now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/568
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/474/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #568: TS-4072 Diagnostic log rolling races
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/568
Time to land this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59889953
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
return false;
- if (stdout_log) {
- delete stdout_log;
- stdout_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stdout_log = new BaseLogFile(_bind_stdout);
+ BaseLogFile *old_stdout_log = stdout_log;
+ BaseLogFile *new_stdout_log = new BaseLogFile(_bind_stdout);
// on any errors we quit
- if (!stdout_log || stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
+ if (!new_stdout_log || new_stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
fprintf(stderr, "[Warning]: unable to open file=%s to bind stdout to\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- if (!stdout_log->m_fp) {
+ if (!new_stdout_log->m_fp) {
fprintf(stderr, "[Warning]: file pointer for stdout %s = NULL\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- return rebind_stdout(fileno(stdout_log->m_fp));
+ // now exchange the stdout_log pointer
+ lock();
+ stdout_log = new_stdout_log;
+ bool ret = rebind_stdout(fileno(new_stdout_log->m_fp));
--- End diff --
I think there's a couple of issues here. First, if the error can't (or shouldn't) reasonably be handled by the caller, then there's little value in trying to propagate it.
``fileno()`` can't fail unless there's a programmer error. and `dup2()`` can't fail unless there is a programmer error or we run out of file descriptors. If you look at ``rebind_stderr()``, we don't even propagate the return value of ``dup2()``.
I think that this should be handled by a ``ink_release_assert``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/568#issuecomment-219191875
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59631163
--- Diff: lib/ts/Diags.cc ---
@@ -853,29 +867,39 @@ Diags::set_stderr_output(const char *_bind_stderr)
if (strcmp(_bind_stderr, "") == 0)
return false;
- if (stderr_log) {
- delete stderr_log;
- stderr_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stderr_log = new BaseLogFile(_bind_stderr);
+ BaseLogFile *old_stderr_log = stderr_log;
+ BaseLogFile *new_stderr_log = new BaseLogFile(_bind_stderr);
// on any errors we quit
- if (!stderr_log || stderr_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
+ if (!new_stderr_log || new_stderr_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
fprintf(stderr, "[Warning]: unable to open file=%s to bind stderr to\n", _bind_stderr);
- delete stderr_log;
+ fprintf(stderr, "[Warning]: stderr is currently not bound to anything\n");
+ delete new_stderr_log;
+ lock();
stderr_log = NULL;
+ unlock();
return false;
}
- if (!stderr_log->m_fp) {
+ if (!new_stderr_log->m_fp) {
--- End diff --
``new_stderr_log->is_open()``
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4072 Diagnostic log rolling races
Posted by danobi <gi...@git.apache.org>.
Github user danobi commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/568#discussion_r59888075
--- Diff: lib/ts/Diags.cc ---
@@ -816,29 +820,39 @@ Diags::set_stdout_output(const char *_bind_stdout)
if (strcmp(_bind_stdout, "") == 0)
return false;
- if (stdout_log) {
- delete stdout_log;
- stdout_log = NULL;
- }
-
- // create backing BaseLogFile for stdout
- stdout_log = new BaseLogFile(_bind_stdout);
+ BaseLogFile *old_stdout_log = stdout_log;
+ BaseLogFile *new_stdout_log = new BaseLogFile(_bind_stdout);
// on any errors we quit
- if (!stdout_log || stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
+ if (!new_stdout_log || new_stdout_log->open_file() != BaseLogFile::LOG_FILE_NO_ERROR) {
fprintf(stderr, "[Warning]: unable to open file=%s to bind stdout to\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- if (!stdout_log->m_fp) {
+ if (!new_stdout_log->m_fp) {
fprintf(stderr, "[Warning]: file pointer for stdout %s = NULL\n", _bind_stdout);
- delete stdout_log;
+ fprintf(stderr, "[Warning]: stdout is currently not bound to anything\n");
+ delete new_stdout_log;
+ lock();
stdout_log = NULL;
+ unlock();
return false;
}
- return rebind_stdout(fileno(stdout_log->m_fp));
+ // now exchange the stdout_log pointer
+ lock();
+ stdout_log = new_stdout_log;
+ bool ret = rebind_stdout(fileno(new_stdout_log->m_fp));
--- End diff --
It should be fine.
At that point in code, `m_fp` is guaranteed to be pointing to some open file. If `rebind_stdout()` fails, it's because (1) `fileno()` failed or (2) `dup2()` failed. If either of those happen, then there's really nothing we can do about it (AFAICT). So ATS will just plod along without `stdout` or `stderr` being bound to anything. Fortunately nobody should be writing directly to either of those streams so we should be in reasonable shape even if `rebind_...()` fails. Hopefully the next time the log files roll the error will correct itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---