Skip to content

cli: use the right context for logging on signal#26716

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:signal-ctx
Jun 14, 2018
Merged

cli: use the right context for logging on signal#26716
craig[bot] merged 1 commit intocockroachdb:masterfrom
andreimatei:signal-ctx

Conversation

@andreimatei
Copy link
Contributor

We were using a long-gone context with a potentially finished span,
leading to log-after-finish crashes.

Fixes #26715

Release note: None

We were using a long-gone context with a potentially finished span,
leading to log-after-finish crashes.

Fixes cockroachdb#26715

Release note: None
@andreimatei andreimatei requested a review from a team as a code owner June 13, 2018 23:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Jun 14, 2018

this may need backporting

@andreimatei
Copy link
Contributor Author

the problem was introduced after 2.0

bors r+


Review status: :shipit: complete! 0 of 0 LGTMs obtained


Comments from Reviewable

craig bot pushed a commit that referenced this pull request Jun 14, 2018
26716: cli: use the right context for logging on signal r=andreimatei a=andreimatei

We were using a long-gone context with a potentially finished span,
leading to log-after-finish crashes.

Fixes #26715

Release note: None

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@knz
Copy link
Contributor

knz commented Jun 14, 2018

the problem was introduced after 2.0

I see a call to stopper.Stop(ctx) in the log.FatalChan() case in start.go (v2.0). Shouldn't this use shutdownCtx instead?

@craig
Copy link
Contributor

craig bot commented Jun 14, 2018

Build succeeded

@craig craig bot merged commit 80afdb8 into cockroachdb:master Jun 14, 2018
@andreimatei andreimatei deleted the signal-ctx branch June 14, 2018 15:42
@andreimatei
Copy link
Contributor Author

I see a call to stopper.Stop(ctx) in the log.FatalChan() case in start.go (v2.0). Shouldn't this use shutdownCtx instead?

Ah indeed, but that's a different enough bug. Will send a fix for that too.

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.

5 participants

Comments