jonhoo / tracing-timing

Inter-event timing metrics on top of tracing.
113 stars 11 forks source link

`LockResult` is unwrapped in `drop_span`, even when unwinding #7

Closed hawkw closed 5 years ago

hawkw commented 5 years ago

While skimming through the code this morning, I noticed that the drop_span implementation has two places where unwrap is called on a LockResult: https://github.com/jonhoo/tracing-timing/blob/9d48c4c80594ea239541ee7e37cc263635dbd047/src/lib.rs#L554

https://github.com/jonhoo/tracing-timing/blob/9d48c4c80594ea239541ee7e37cc263635dbd047/src/lib.rs#L559

Although panicking when a lock is poisoned by another thread is correct, drop_span is called from within the Drop implementation for tracing::Span, and thus can be called when the thread is already unwinding. Therefore, a panic in drop_span can result in a double panic, leading to the process being aborted abruptly rather than displaying a backtrace/panic message.

Instead, drop_span should probably do nothing if the thread is panicking --- since updating the reference counts is mostly responsible for reclaiming memory in this case, it's okay to just return as the process is about to be aborted by the panic regardless.

To still propagate panics when the thread is not already unwinding, the best solution is probably to just add

if std::thread::panicking() {
    return;
}

to the top of drop_span.