feat: Replace forest write lock Channel with Semaphore #659
Conversation
snowmead
added
B5-clientnoteworthy
…k-semaphore # Conflicts: # client/blockchain-service/src/handler_bsp.rs
snowmead
force-pushed
the
feat/forest-write-lock-semaphore
branch
from
a9b0b3b to
405aeee
Compare
ffarall
left a comment
ffarall
left a comment
There was a problem hiding this comment.
Very neat. Love the way this ended up looking.
Besides some comments, I want to understand why this hasn't had an impact on any of our integration tests. Didn't anything release faster? And if that's not the case, could we make such a test case that evidences that this releases the lock faster on failure cases?
Anyway, not fully approving yet, just to merge other PRs first. Although this is not breaking, it is a change in one of the core functionalities of BSPs and MSPs, and I'd like it to be tested for some time in Stagenet.
| fn drop(&mut self) { | ||
| // Send notification to the blockchain service event loop to process pending requests. | ||
| // Ignore errors as the receiver may be dropped during shutdown. | ||
| let _ = self.release_notifier.send(()); |
There was a problem hiding this comment.
I think this merits at least a log if this fails. Just so that we at least know something is off.
| // This avoids a subtle event-loop spin: acquiring + immediately dropping the permit | ||
| // would notify `permit_release_receiver`, which would call back into this method | ||
| // even though there is no work to do. | ||
| let has_pending_work = { |
There was a problem hiding this comment.
Instead of doing this "pre-check" to check for pending work, and then acquiring the lock, why not using the next_event_data variable that is instantiated later, that will tell you if there is pending work or not, and only then try to acquire the lock.
My understanding is that when you checked bsp_handler.forest_root_write_semaphore.available_permits() you already checked that the lock is available.
| // This avoids a subtle event-loop spin: acquiring + immediately dropping the permit | ||
| // would notify `permit_release_receiver`, which would call back into this method | ||
| // even though there is no work to do. | ||
| let has_pending_work = { |
There was a problem hiding this comment.
Instead of doing this "pre-check" to check for pending work, and then acquiring the lock, why not using the next_event_data variable that is instantiated later, that will tell you if there is pending work or not, and only then try to acquire the lock.
My understanding is that when you checked bsp_handler.forest_root_write_semaphore.available_permits() you already checked that the lock is available.
Replaces the forest root write lock channel with a semaphore.
The goal is to simplify the locking mechanism for tasks to acquire them and release them in any eventuality (i.e. when the task either succeeds, errors out or panics) without having to manually release the lock within the event handler.
A RAII guard with a channel sender is passed to the task event handlers so when the event itself is dropped, the guard executes it's
dropimplementation and sends a unit value()to theBlockchainServiceEventLoopreceiver channel to reassign the lock to another event.With this approach we remove all boilerplate locking semantics in the event handlers, and ensure that, in any case, the forest write lock reassigns to pending events waiting on the lock immediately.