PS-11143-[8.4] sql/auth: quiesce caching_sha2_password before teardown#6028
PS-11143-[8.4] sql/auth: quiesce caching_sha2_password before teardown#6028lukin-oleksiy wants to merge 1 commit into
Conversation
a9ba74f to
13101d4
Compare
jankowsk
left a comment
There was a problem hiding this comment.
That solves the problem of unloading the plugin while an authentication is ongoing. It
would work providing the authentication is never called after deinit. Are we sure this condition is met?
|
Generally, how it works (8.4): do_auth_once() locks the auth plugin (my_plugin_lock_by_name(...)), calls auth->authenticate_user(...), then unlocks it (plugin_unlock(...)) in sql/auth/sql_authentication.cc (around lines 3535-3557). So, conclusion is: we are safe when unloading is complete. |
|
Hmmm... Then actually your fix should not be necessary, because the described mechanism does the same thing, but at higher level. Which means the problem is probably in something else. |
13101d4 to
551996c
Compare
Add shutdown protection on the level into the shared auth/plugin lifecycle so all authentication plugins are covered. - Add a global auth-plugin shutdown barrier API (`sql/auth/auth_plugin_shutdown.h` + implementation in `sql/auth/sql_authentication.cc`). - Guard auth plugin callback entry points with RAII operation tracking. - Start barrier shutdown/drain from `plugin_shutdown()` before plugin deinitialization (`sql/sql_plugin.cc`). - Remove plugin-local quiesce state/wait logic from `sql/auth/sha2_password.cc` now that teardown protection is centralized. Regression coverage: - Add `percona.bug_ps11143_threadpool_auth_shutdown` to reproduce the threadpool auth-vs-shutdown race and verify clean shutdown. - Include expected result file and test cleanup to preserve MTR state. Verified: - percona.bug_ps11143_threadpool_auth_shutdown - percona.signal_handling_threadpool - percona.threadpool_stats - percona.kill_idle_trx_threadpool - percona.threadpool_debug
551996c to
8d4ded2
Compare
|
I moved the shutdown protection a level up, but the fix is more "invasive". |
jankowsk
left a comment
There was a problem hiding this comment.
The static/global shutdown state variable brings some risk of at destruction (it might crash if the mutex is locked or any thread is waiting on the conditional). I was not able to construct a such scenario, but please consider it :-).
|
|
||
| namespace { | ||
|
|
||
| class Auth_plugin_shutdown_state { |
There was a problem hiding this comment.
Why this is defined here? It is rather tightly related to Auth_plugin_operation_guard.
In fact, some details of this state should be visible only to the guard class and we need only a single instance of the shutdown status class to be used via the guard class. In order to better express intentions, avoid misuse and possibly simplify the code I'd suggest:
- replacing the below global g_auth_plugin_shutdown_state by a static member of Auth_plugin_operation_guard (the static functions will be to remove)
- making Auth_plugin_operation_guard friend of Auth_plugin_shutdown_state and constructors of the latter protected
- moving the class implementation to e.g. sql/auth/auth_plugin_shutdown.cc
There was a problem hiding this comment.
Please, remove formatting not relevant to the fix. Does this file have to be modified at all?
| if (--m_active_operations == 0) m_cv.notify_all(); | ||
| } | ||
|
|
||
| void start_shutdown_and_wait() { |
There was a problem hiding this comment.
I'm wondering what happens when:
- A new connection starts authenticating
- The server starts closing
Please note, that some authentications may take a while (e.g. IIRC LDAP takes few client - server exchanges).
If the threads are stopped first (seems to be more logical to me), how it is done, does the main wait until all connections are "gently" closed? I suppose at least there is some timeout. Then (I guess) sth like pthread_cancel() may be used and if so, probably m_active_operations never reach 0 and the server hangs. For that scenario I recommend replacing wait() with wait_for().
If the order is reverse, the hank is even more obvious.
https://perconadev.atlassian.net/browse/PS-11143
Add shutdown protection on the level into
the shared auth/plugin lifecycle so all authentication plugins are covered.
(
sql/auth/auth_plugin_shutdown.h+ implementation insql/auth/sql_authentication.cc).plugin_shutdown()before plugindeinitialization (
sql/sql_plugin.cc).sql/auth/sha2_password.ccnow that teardown protection is centralized.Regression coverage:
percona.bug_ps11143_threadpool_auth_shutdownto reproduce thethreadpool auth-vs-shutdown race and verify clean shutdown.
Verified: