Repository navigation
[logger] Force sequential writes to gRPC handler - #5384
Conversation
The gRPC WriterInterface::Write function is only stated to be thread-safe with respect to ::Read. Concurrent writes can cause an abort in gRPC code.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The mutex is not shared with other response writes, so concurrent gRPC Write calls can still occur.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds locking to serialize client logger gRPC writes and includes a concurrency regression test.
Changes:
- Adds a mutex around
ClientLogger::Write. - Adds a unit test for concurrent logging.
| File | Summary |
|---|---|
tests/unit/test_client_logger.cpp |
Tests non-overlapping concurrent writes. |
include/multipass/logging/client_logger.h |
Serializes logger writes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::lock_guard lock{write_mutex}; | ||
| server->Write(reply); |
There was a problem hiding this comment.
I think this is a valid concern, but outside the scope of this PR. The Multipass daemon does not yet fully support concurrent requests making this scenario currently rare.
There was a problem hiding this comment.
Agreed. This makes it better.
@theartful highlighting this as something to take into account in your efforts: gRPC server writes need to be sequential.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5384 +/- ##
==========================================
- Coverage 71.35% 71.33% -0.01%
==========================================
Files 347 347
Lines 19056 19044 -12
==========================================
- Hits 13595 13583 -12
Misses 5461 5461 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ricab
left a comment
ricab
left a comment
There was a problem hiding this comment.
Yup, very good. I have a small suggestion inline, but approving anyway.
| int in_flight = 0; | ||
| bool overlapped = false; | ||
|
|
||
| EXPECT_CALL(mock_srw, Write(testing::_, testing::_)) |
There was a problem hiding this comment.
Can't the args be omitted? It should be possible if there is no overload ambiguity, IIRC.
Otherwise, most other test files just use namespace testing. Might be a good chance to align here.
| std::lock_guard lock{write_mutex}; | ||
| server->Write(reply); |
There was a problem hiding this comment.
Agreed. This makes it better.
@theartful highlighting this as something to take into account in your efforts: gRPC server writes need to be sequential.
|
@sharder996 I think this should have gone into the release branch, I forgot to check that. @tobe2098 looks like a late cherry-pick. |
Description
The gRPC WriterInterface::Write function is only stated to be thread-safe with respect to ::Read. Concurrent writes can cause an abort in gRPC code. This PR adds a lock to protect writes to the client logger through the
grpc::ServerReaderWriterInterface.Related Issue(s)
Fixes #3578
Testing
Follow the reported issue to reproduce the crash.
Checklist
MULTI-2947