fix: access to wrong thread local after command migrates fiber to a different thread #2410
Conversation
|
@romange @dranikpg IMO, I do not like the idea to introduce p.s. The very reason that I am not happy with |
src/server/main_service.cc
Outdated
| // We are not sending any admin command in the monitor, and we do not want to | ||
| // do any processing if we don't have any waiting connections with monitor | ||
| // enabled on them - see https://redis.io/commands/monitor/ | ||
| const MonitorsRepo& monitors = etl.Monitors(); | ||
| if (!monitors.Empty() && (cid->opt_mask() & CO::ADMIN) == 0) { | ||
| if (!ServerState::SafeTLocal()->Monitors().Empty() && (cid->opt_mask() & CO::ADMIN) == 0) { |
There was a problem hiding this comment.
please add a comment explaining the context behind SafeTLocal call
src/server/server_state.h
Outdated
| static ServerState* tlocal() { | ||
| return state_; | ||
| } | ||
|
|
||
| // Safe version. https://stackoverflow.com/a/75622732 |
There was a problem hiding this comment.
please explain here what Safe means. I do not think that this stack overflow link applies here (asm volatile is not needed). because we always use it after the call to a function that can not be inlined (Invoke()), hence a compiler won't be able to reorder a call to SafeTLocal to before Invoke.
There was a problem hiding this comment.
I think we should keep the link only in the implementation, the answer is too long and sophisticated to read for anyone reading the code (or is it just me 😆), instead we can describe in one sentence what safe means
src/server/main_service.cc
Outdated
| invoke_time_usec >= etl.log_slower_than_usec) { | ||
| if (!(cid->opt_mask() & CO::BLOCKING) && conn != nullptr && | ||
| // Use SafeTLocal() to avoid accessing the wrong thread local instance | ||
| ServerState::SafeTLocal()->GetSlowLog().IsEnabled() && |
There was a problem hiding this comment.
one last nit.
I think it we will more elegant and more performant that instead of
checking both IsEnabled() and invoke_time_usec >= ServerState::SafeTLocal()->log_slower_than_usec
we will introduce bool ServerState::ShouldLogSlowCmd(unsigned latency_usec) const that will do both at the same time.
The issue is that within
Service::InvokeCmd, we copy to a local variable theServerState::tlocal()before we invoke the command. After the invocation, it could be the case that the connection migrated. If that's true, accessing the local variable creates a data race (because the thread local variable leaked to a different thread, and now it can be accessed concurrently)