enh/793: add read-write lock to yamuxstream #897
Conversation
|
@codemaestro64 : Wish if you could study the implementation in mplex stream first: please visit #748. Then start the implementation in yamux stream. CCing @acul71, who is guiding you on this PR. Adding @Jineshbansal and @lla-dane, who could share reviews as you proceed with the PR. |
libp2p/stream_muxer/yamux/yamux.py
Outdated
| @@ -80,6 +83,8 @@ def __init__(self, stream_id: int, conn: "Yamux", is_initiator: bool) -> None: | |||
| self.send_window = DEFAULT_WINDOW_SIZE | |||
| self.recv_window = DEFAULT_WINDOW_SIZE | |||
| self.window_lock = trio.Lock() | |||
| self.rw_lock = ReadWriteLock | |||
There was a problem hiding this comment.
Did you forget a () ?
self.rw_lock = ReadWriteLock()
|
@codemaestro64, your approach follows the same path Go libp2p took. Go had similar concurrency issues with yamux and initially solved them with read-write locks before optimizing with atomic operations. Go Implementation Evolution
Recommendation
|
Thank you @seetadev Studied the mplex stream implementation. |
Thanks for the detailed feedback, @acul71! I’ve addressed the following from your recommendations:
Should I proceed with implementing atomic operations in this PR? |
codemaestro64
changed the title
There was a problem hiding this comment.
Hello @codemaestro64 Well done.
Just resolve the pytest_configure and it's good for merge
tests/conftest.py
Outdated
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def security_protocol(): | ||
| return None | ||
|
|
||
|
|
||
| def pytest_configure(config): |
There was a problem hiding this comment.
Did you add this pytest_configure function for debugging?
Is still necessary?
Because I don't like this function to be defined here globally for all test, with specific yamux logging conf.
I think better get rid of this or eventually move it in tests/core/stream_muxer/conftest.py
There was a problem hiding this comment.
Hello @acul71 Yes, the pytest_configure function was added for debugging purposes. It's been removed in the latest commit.
…urrent read/write corruption Introduce a read/write lock abstraction and integrate it into `YamuxStream` so that simultaneous reads and writes do not interleave, eliminating potential data corruption and race conditions. Major changes: - Abstract `ReadWriteLock` into its own util module - Integrate locking into YamuxStream for `write` operations - Ensure tests pass for lock correctness - Fix lint & type issues discovered during review Closes libp2p#793
codemaestro64
force-pushed
the
enhancement/yamuxstream-lock
branch
from
03b2d30 to
c5a2836
Compare
|
@codemaestro64 : Great effort. Could you please reply to @acul71 's feedback. Also, please resolve the CI/Cd issues. @acul71 : Thank you for reviewing the PR and for sharing detailed feedback. Appreciate your great support as always. |
|
@codemaestro64 : Please also add a test suite and newsfragment. Looking forward to final review soon. |
Hi @seetadev These have already been done |
What was wrong?
Issue #793:
Concurrent read and write operations on the same
Yamuxstream can interleave, leading to potential data corruption and race conditions.The
YamuxStreamclass currently lacks a dedicated concurrency lock to ensure safe concurrent read and write access.How was it fixed?
As the first step toward resolving this, the
ReadWriteLockimplementation was abstracted into a reusable utility module (stream_muxer/rw_lock.py).This allows it to be cleanly reused across different parts of the codebase, including
yamuxand potentially other stream muxers.To-Do
ReadWriteLockinto a reusable module (utils/rw_lock.py)ReadWriteLockintoYamuxStreamto prevent concurrent read/write corruptioncloses #793