Skip to content

morph::log offers no noexcept guarantee, so every noexcept caller hand-rolls a try/catch — make the logging layer non-throwing #158

Description

@Yaraslaut

The gap

morph::log's logging calls can throw, and docs/spec/core/logger.md documents this as deliberate:

A throwing sink propagates. Exceptions from the sink are not caught. They unwind out of detail::log […] and out of the originating logDebug/logInfo/… call into the caller. The logging layer adds no try/catch; a sink that must not disrupt its caller has to swallow its own exceptions internally.

That contract puts the burden on the sink. The problem is that a caller cannot rely on it: whether logging throws depends on which sink some other part of the program installed, and a library function in a noexcept context has no way to know or control that.

It is also not only the sink. detail::log takes a std::scoped_lock on LogState::mtx, and std::mutex::lock can throw std::system_error — so even with a perfectly well-behaved sink, no logging call is noexcept today.

This is already a live hazard, already worked around

CompletionState's destructor (include/morph/core/completion.hpp:149) logs orphaned exceptions. A destructor is implicitly noexcept, so a throwing logger there means std::terminate. The code already knows:

// NOLINTBEGIN(bugprone-empty-catch) — logError may throw; we swallow to avoid noexcept-escape
try {
    std::rethrow_exception(error);
} catch (const std::exception& exc) {
    try {
        ::morph::log::logError("[orphan] unhandled exception: " + std::string{exc.what()});
    } catch (...) {
    }
}

A nested try/catch(...) around a log call, plus a bugprone-empty-catch suppression, purely because the logging layer offers no guarantee. That is the workaround this issue proposes to make unnecessary.

Why it matters beyond that one site

The same shape recurs wherever a noexcept function wants to log. In #153 (Rational's saturating arithmetic) the operators must log on overflow; keeping them noexcept requires the identical local try/catch, or dropping noexcept from arithmetic operators that had it. Neither is a good trade for "we wanted to write a log line".

This is the same class of problem as #138 (every caller hand-rolls a liveness token) and #96/#97 (every caller needs its own fault seam): a guarantee that belongs in one place, currently reimplemented per call site — and easy to forget, because forgetting compiles.

Suggested direction

Make the logging layer non-throwing, and say so in the type system:

  • detail::log(LogLevel, std::string_view) noexcept — wrap the lock acquisition and the sink invocation in try { … } catch (...) {}.
  • The public helpers (logDebug/logInfo/logWarn/logError, both the string_view and variadic overloads) become noexcept too. The variadic ones must also guard std::format, which can throw std::bad_alloc.
  • Update docs/spec/core/logger.md's "Failure modes" section, replacing the "a throwing sink propagates" clause.
  • Drop the local workaround in CompletionState's destructor, and its NOLINT.

The trade-off worth stating explicitly: swallowing sink exceptions hides bugs in the sink. That is a real cost. But the alternative is that a logging call can take down an unrelated caller — or std::terminate a process from a destructor — and the spec already constrains sinks heavily (it tells them not to re-enter morph::log, and not to block). Treating "the sink misbehaved" as a logging-layer concern rather than a caller concern is consistent with that. If swallowing silently is unpalatable, a counter counting dropped log records would preserve the signal without propagating.

Alternative, if the current contract is intentional and should stand

If sink-propagation is genuinely wanted, then the honest fix is the opposite: document that morph::log must never be called from a noexcept context or a destructor, and audit the existing call sites for violations. CompletionState's destructor is one today. I do not think that is the better option — logging from teardown paths is exactly where it is most useful — but it is a coherent one, and the current state is neither.

Context

Found while implementing #153, where Rational's arithmetic operators need to log on overflow and would otherwise lose noexcept. That PR works around it locally the same way CompletionState does; the workaround can be removed if this lands.

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: coreSubsystem: coretriage: validWell-framed; implement as written

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions