fix(AclFamily): stream acl updates via dispatch queue in connection #1786
Conversation
There was a problem hiding this comment.
I really wish for a test. For example, make a long running script that jumps on many shards. This means that the control fiber will suspend at some time and StreamUpdatesToAllProactor will have a change to run. Then we make sure that the acl_categories didn't change while the script was running.
| if (ctx && msg.username == ctx->authed_username) { | ||
| ctx->acl_categories = msg.categories; | ||
| } |
There was a problem hiding this comment.
I first thought that you can send this message to a connection only if it already has this specific user. However if a acl setuser operation manages to execute before it it whould've been a bug. So its correct this way
There was a problem hiding this comment.
Technically the set acluser must complete, since it holds a write lock over the registry.
| struct AclUpdateMessage { | ||
| std::string_view username; | ||
| uint64_t categories{0}; | ||
| }; |
There was a problem hiding this comment.
Please add a comment above why we have it at all
There was a problem hiding this comment.
And quick check: are you sure the string_view keeps valid forever? The connection might be very busy and process this message only after a long delay
There was a problem hiding this comment.
Yes I am sure, because we hold a ReadLock on the registry so nobody can actually modify it:
180 auto& registry = ServerState::tlocal()->user_registry;
181 auto user_with_lock = registry->MaybeAddAndUpdateWithLock(username, std::move(req));
182 if (user_with_lock.exists) {
183 StreamUpdatesToAllProactorConnections(username, user_with_lock.user.AclCategory());
184 }
There was a problem hiding this comment.
which is a bug! I will fix it, we need a unique_lock!
| @pytest.mark.asyncio | ||
| async def test_acl_with_long_running_script(df_server): | ||
| client = aioredis.Redis(port=df_server.port) | ||
| await client.execute_command("ACL SETUSER roman ON >yoman +@string +@scripting") |
There was a bug on updates of the acl categories when squashing was used. Basically, the parent context could be accessed in parallel by the "stub" contexts causing a dreaded data race on the update.
This is fixed by adding a new
AclUpdateMessageat the front of the dispatch queue of the connection.