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

cherrypick-1.1: util/log: don't panic #19287

Merged
merged 1 commit into from
Oct 16, 2017

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Oct 16, 2017

Cherry-pick of #17871. Fixes, most likely, #19202.

cc @cockroachdb/release


Previously, log.outputLogEntry could panic while holding the log mutex.
This would deadlock any goroutine that logged while recovering from the
panic, which is approximately all of the recover routines. Most
annoyingly, the crash reporter would deadlock, swallowing the cause of
the panic.

Avoid panicking while holding the log mutex and use l.exit instead,
which exists for this very purpose. In the process, enforce the
invariant that l.mu is held when l.exit is called. (The previous
behavior was, in fact, incorrect, as l.flushAll should not be called
without holding l.mu.)

Also add a Tcl test to ensure this doesn't break in the future.

@benesch benesch requested a review from a team as a code owner October 16, 2017 16:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@benesch benesch changed the title util/log: don't panic cherrypick-1.1: util/log: don't panic Oct 16, 2017
@dt dt requested review from tbg and removed request for tbg October 16, 2017 16:40
Previously, log.outputLogEntry could panic while holding the log mutex.
This would deadlock any goroutine that logged while recovering from the
panic, which is approximately all of the recover routines. Most
annoyingly, the crash reporter would deadlock, swallowing the cause of
the panic.

Avoid panicking while holding the log mutex and use l.exit instead,
which exists for this very purpose. In the process, enforce the
invariant that l.mu is held when l.exit is called. (The previous
behavior was, in fact, incorrect, as l.flushAll should not be called
without holding l.mu.)

Also add a Tcl test to ensure this doesn't break in the future.
@benesch
Copy link
Contributor Author

benesch commented Oct 16, 2017

Needed to apply the following patch to get tests to pass:

diff --git a/pkg/cli/interactive_tests/test_missing_log_output.tcl b/pkg/cli/interactive_tests/test_missing_log_output.tcl
index 58e219737..f1c391c85 100644
--- a/pkg/cli/interactive_tests/test_missing_log_output.tcl
+++ b/pkg/cli/interactive_tests/test_missing_log_output.tcl
@@ -27,7 +27,7 @@ system "grep -F 'log: exiting because of error: write /dev/stderr: broken pipe'
 end_test
 
 start_test "Check that a broken log file prints a message to stderr."
-send "$argv start -s=path=/dev/null --insecure --logtostderr\r"
+send "$argv start -s=path=logs/db --log-dir=/dev/null --insecure --logtostderr\r"
 eexpect "log: exiting because of error: log: cannot create log: open"
 eexpect "not a directory"
 eexpect ":/# "

This is a more sensible test, anyway. I'll send that patch on master.

@benesch
Copy link
Contributor Author

benesch commented Oct 16, 2017

Never mind; @andreimatei already made the equivalent fix on master.

@benesch benesch merged commit f3bc578 into cockroachdb:release-1.1 Oct 16, 2017
@benesch benesch deleted the 1.1-dont-panic branch October 16, 2017 17:27
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.

3 participants