SignalLogger maintains internal state so is not thread safe, even with per-thread Contexts

[edrumwri]

Per discussion with @sherm1 the other day, SignalLogger is maintaining state (the log is mutable and maintained within the system). This violates our contract that Systems do not maintain state.

EDIT (sherm): the most serious consequence of this is that SignalLogger breaks thread safety even if a user is following the per-thread-Context rule. It should likely use the MITLS pattern as a cure.

SignalLogger also uses an O(n²) reallocation scheme that should be replaced with something better.

[jwnimmer-tri]

I disagree that this is a problem. A system whose job it is to interact with code outside of the simulation should feel free to keep local state in this, which is only exposed externally. As long as the x, xdot, y, etc. calculations are not affected by the mutable member field, there is no issue.

[edrumwri]

I disagree with your disagreement. If we have the same system being used in multiple threads, race conditions will result. It's not hard to imagine that happening in this case. Safe parallelism will be straightforward to achieve if we keep the "code" and "data" segregated.

[jwnimmer-tri]

In general, nobody really has the right to assume that any given C++ class is intrinsically threadsafe. That's not how code gets formulated by default. Data races are usually guarded at a higher layer than any single object.

Beyond that, it is not currently a rule (and I don't think it should be a rule) that every System must be implemented in a thread-safe way. When you say "our contract that Systems do not maintain state", what do you mean? I think we agree that the derivatives, discrete updates, outputs, etc. from a System should be deterministically based on the Context. This issue seems to imply more than that, but I don't immediately see anywhere we've documented such restrictions.

[edrumwri]

It's a commonly expressed use case where people want to simulate (or otherwise evaluate) diagrams across multiple threads. While it might not be a rule that every System is implemented in a thread safe way, segregating the code and data is a straightforward way to facilitate that. If that wasn't the rationale for introducing the segregation, I don't know what was.

When you say that "derivatives, updates, outputs, etc. from a System should be deterministically based on the Context", you're pointing out that SignalLogger's log is state: there is no way to look at the (current) context and generate the log data.

[jwnimmer-tri]

You're right that there is sometimes an advantage to having a system be able to execute on multiple threads, and indeed many of our core systems should implement support for such. But it should not be a de jure restriction that every single System must implement always. My disagreement is with the implication that SignalLogger is de-jure defective, rather than pointing out a use case where we need to acutely opt-in to being de-facto thread-safe for this system.

In other words, is this a bug report or feature request? I disagree that it's a bug report. It could be OK as a feature request, but we'd need to see some evidence that we need the feature, before pursuing it.

When you say that "derivatives, updates, outputs, etc. from a System should be deterministically based on the Context", you're pointing out that SignalLogger's log is state: there is no way to look at the (current) context and generate the log data.

I think you misunderstand me. The SignalLogger::data() is not an output (in the System sense)! It's just a method. It doesn't need to be governed by the Context. If it exposed the trajectory on an output port, then the trajectory would need to be saved in the context.

If that wasn't the rationale for introducing the segregation, I don't know what was.

Multithreading is absolutely not the motivation for separating System from Context!

[edrumwri]

I'm standing by my assertion that this is defective. I really don't see how you can argue otherwise- this is a landmine waiting for a user to step on it.

[jwnimmer-tri]

Given that it's been this way since #4257 which is 2+ years old, I think the empirical risk of a user stepping on this is close to zero. If it's such a significant hazard, why has nobody experienced a problem over its lifetime to date?

[edrumwri]

You want to collect statistics over a small N? And that argument might be useful in changing the priority of this issue, but it doesn't speak to whether this is a defect or not.

I assert that this is a defect. There's no documentation about this potential problem, and multithreading is an already-employed use case.

This line of argumentation does not seem consistent with your typical focus on defensive programming practices.

[jwnimmer-tri]

In general, nobody really has the right to assume that any given C++ class is intrinsically threadsafe.

That's my bottom line. Most classes literally cannot defend against users doing stupid things with threads. Programmers must not assume that they can call an object from multiple threads at once and have good things happen. I do not want us to make the mistake of assuming otherwise, when deciding on the contract for what a System should be.

[sherm1]

I'm doing some mild reworking of SignalLogger (currently in WIP #10132 but will be its own PR). My approach there was just to document the egregious non-thread-safety of SignalLogger, which I think is a reasonable accommodation.

I disagree with Jeremy that the System/Context split wasn't motivated by thread safety. As an inventor of that concept, I can state with authority that it was indeed one of the things I like about it. It provides mild thread safety in the sense that if you have one Context per thread, you can share the same System. The current implementation of SignalLogger breaks even that level of thread safety.

There are other things that make SignalLogger as currently implemented somewhat of a toy, so I'm happy just to fix it for now with documentation and warnings.

[jwnimmer-tri]

Oops. I interpreted Evan's "the rationale for introducing the segregation" that he meant only rationale. Indeed, it is one of the points in favor of System-vs-Context, but there are many more important ones.

[sherm1]

An easy and almost-compatible fix for this might be to provide an option for the user to provide SignalLogger a thread_local matrix in which to hold the log.

[sherm1]

The SignalLogger thread-safety problem is not fixed but is at least now documented in PR #10269. An additional problem is that it uses an O(n²) reallocation scheme. Both problems gain TODOs in #10269.

[jwnimmer-tri]

Per RobotLocomotion/styleguide#46, one resolution here would be to log into a scratch CacheEntry in the Context. In that case all of the getters like num_samples, data, etc. would gain a Context<T> as an argument.

[jwnimmer-tri]

The code still has a TODO comment citing this issue that says SignalLogger is defective. I think that's stale?

drake/systems/primitives/signal_logger.h

Lines 18 to 19 in 3c4e32f

	// TODO(sherm1) This System has problems as discussed in issue #10228 that
	// should be fixed.

[jwnimmer-tri]

Ugh, my bad. I got mixed up about which file was old and which file was new.