Lock access to the plugin loading channels #13682
Conversation
Changelog[uncommitted] (2023-08-10)Bug Fixes
|
Frassle
force-pushed
the
fraser/pluginCloseSync
branch
from
b0a84c8
to
0220473
Compare
There was a problem hiding this comment.
Yeah, this looks like it'll work.
I think we might be able to do a not-super-complicated test for this:
call loadPlugin with a no-op load and Close concurrently, and it'll probably panic some times without this fix.
(If it's more complicated than that, ignore this part of the comment.)
Yup that panic'd it. |
Frassle
force-pushed
the
fraser/pluginCloseSync
branch
from
0220473
to
18f1f8a
Compare
|
bors merge |
|
merge will fail. lint is missing t.Parallel. I can handle it in half an hour if you're done for the day. |
|
🤦♂️ Fixing.... |
Fixes #12371. This locks access to the plugin request channels with a RWLock. Before trying to write to the channel we try to take a Read lock (yes this sounds the wrong way round, carry on). Many loaders are free to send to the loadRequest channel at once, but we use the read lock to atomiclly track if any are currently in progress. When we go to close the plugin host the first thing we do is take a Write lock. Firstly this can't be taken until all the read locks are released indicating that no plugins are currently loading, but secondly while the write lock is taken no more read locks can be taken blocking any further plugin loads from starting. We never release this write lock, thus permenatly blocking plugin loads once `Close` is called. So that we don't indefinently block inside load calls we do a `TryRLock` and return an error if the read lock can't be taken. With this locking in place the rest of `Close` is then free to shut down all current plugins and close of the request channels, assured that they shouldn't be posted to again as the lock stays held.
Frassle
force-pushed
the
fraser/pluginCloseSync
branch
from
18f1f8a
to
8cc9fbd
Compare
|
Canceled. |
|
bors merge |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
Description
Fixes #12371.
This locks access to the plugin request channels with a RWLock. Before trying to write to the channel we try to take a Read lock (yes this sounds the wrong way round, carry on). Many loaders are free to send to the loadRequest channel at once, but we use the read lock to atomiclly track if any are currently in progress.
When we go to close the plugin host the first thing we do is take a Write lock. Firstly this can't be taken until all the read locks are released indicating that no plugins are currently loading, but secondly while the write lock is taken no more read locks can be taken blocking any further plugin loads from starting.
We never release this write lock, thus permenatly blocking plugin loads once
Closeis called. So that we don't indefinently block inside load calls we do aTryRLockand return an error if the read lock can't be taken.With this locking in place the rest of
Closeis then free to shut down all current plugins and close of the request channels, assured that they shouldn't be posted to again as the lock stays held.Checklist
make tidyto update any new dependenciesmake lintto verify my code passes the lint checkgofumptmake changelogand committed thechangelog/pending/<file>documenting my change