[#7305] fixed deadlock when manually triggering the OnTerminate hook

Co-authored-by: Felix <FelixM@yer.tools>

CHANGELOG.md

@@ -1,23 +1,25 @@
 ## v0.32.0 (WIP)
 
 - ⚠️ Added extra List/Search API rules checks for the client-side `filter`/`sort` relations.
+
     This is continuation of the effort to eliminate the risk of information disclosure _(and eventually the side-channel attacks that may originate from that)_.
 
-    So far this was accepted tradeoff between performance, usability and correctness since the solutions at the time weren't really viable _(especially with the back-relations as mentioned in ["Security and performance" section in #4417](https://github.com/pocketbase/pocketbase/discussions/4417))_, but with v0.23+ changes we can implement the extra checks with very little impact on the performance without littering the code too much and at the same time ensuring better out of the box security _(especially for the cases where people operates with non-hidden "code", "token" or other sensitive fields)_.
+    So far this was accepted tradeoff between performance, usability and correctness since the solutions at the time weren't really practical _(especially with the back-relations as mentioned in ["Security and performance" section in #4417](https://github.com/pocketbase/pocketbase/discussions/4417))_, but with v0.23+ changes we can implement the extra checks with very little impact on the performance without littering the code too much and at the same time ensuring better out of the box security _(especially for the cases where users operate with sensitive fields like "code", "token", "secret", etc.)_.
 
-    Similar to the previous release, probably for most users with already configured API rules this change won't be breaking, but if you have an intermediate/junction collection that is "locked" (superusers-only) we no longer will allow the client-side relation filter to pass though it and you'll have to set a List/Search API rule to enable the current user to access it.
+    Similar to the previous release, probably for most users with already configured API rules this change won't be breaking, but if you have an _intermediate/junction collection_ that is "locked" (superusers-only) we no longer will allow the client-side relation filter to pass through it and you'll have to set its List/Search API rule to enable the current user to search in it.
 
     For example, if you have a client-side filter that targets `rel1.rel2.token`, the client must have not only List/Search API rule access to the main collection but also to the collections referenced by "rel1" and "rel2" relation fields.
 
     Note that this change is only for the **client-side** `filter`/`sort` and doesn't affect the execution of superusers requests, API rules and `expand` - they continue to work the same as it is.
 
-    An optional environment variable to toggle this behavior was considered but for now I think having 2 ways of resolving client-side filters would introduce maintenance burden and can even cause confusion (this change should actually make things more intuitive and clear because we can simply say something like _"you can search by a collection X field only if you have List/Search API rule to it"_ no matter whether the targeted collection is the request's main collection, the first or last relation from the filter chain, etc.).
+    An optional environment variable to toggle this behavior was considered but for now I think having 2 ways of resolving client-side filters would introduce maintenance burden and can even cause confusion (this change should actually make things more intuitive and clear because we can simply say something like _"you can search by a collection X field only if you have List/Search API rule access to it"_ no matter whether the targeted collection is the request's main collection, the first or last relation from the filter chain, etc.).
 
     If you stumble on an error or extreme query performance degradation as a result of the extra checks, please open a Q&A discussion with the failing request and export of your collections configuration as JSON (_Settings > Export collections_) and I'll try to investigate it.
 
-
 - Increased the default SQLite `PRAGMA cache_size` to ~32MB.
 
+- Fixed deadlock when manually triggering the `OnTerminate` hook ([#7305](https://github.com/pocketbase/pocketbase/pull/7305); thanks @yerTools).
+
 - Fixed some code comment typos and regenerated the JSVM types.
 
 

core/base.go

@@ -1408,7 +1408,7 @@ func getLoggerMinLevel(app App) slog.Level {
 func (app *BaseApp) initLogger() error {
 	duration := 3 * time.Second
 	ticker := time.NewTicker(duration)
-	done := make(chan bool)
+	done := make(chan bool, 1)
 
 	handler := logger.NewBatchHandler(logger.BatchOptions{
 		Level:     getLoggerMinLevel(app),
@@ -1479,7 +1479,11 @@ func (app *BaseApp) initLogger() error {
 
 			ticker.Stop()
 
-			done <- true
+			// don't block in case OnTerminate is triggered more than once
+			select {
+			case done <- true:
+			default:
+			}
 
 			return e.Next()
 		},

core/base_test.go

@@ -552,3 +552,19 @@ func TestBaseAppAuxDBDualBuilder(t *testing.T) {
 		}
 	}
 }
+
+func TestBaseAppTriggerOnTerminate(t *testing.T) {
+	t.Parallel()
+
+	app, _ := tests.NewTestApp()
+	defer app.Cleanup()
+
+	event := new(core.TerminateEvent)
+	event.App = app
+
+	// trigger OnTerminate multiple times to ensure that it doesn't deadlock
+	// https://github.com/pocketbase/pocketbase/pull/7305
+	app.OnTerminate().Trigger(event)
+	app.OnTerminate().Trigger(event)
+	app.OnTerminate().Trigger(event)
+}

pocketbase.go

@@ -205,6 +205,8 @@ func (pb *PocketBase) Execute() error {
 	<-done
 
 	// trigger cleanups
+	//
+	// @todo consider skipping and just call the finalizer in case OnTerminate was already invoked manually?
 	event := new(core.TerminateEvent)
 	event.App = pb
 	return pb.OnTerminate().Trigger(event, func(e *core.TerminateEvent) error {