Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRE: avoid assert failure in sub-processes #1766

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Apr 8, 2024

Will allow frontend to disable CRE finalizers when running code in a subprocess and explicitely exiting quickly without any proper cleanup.
See #1679 (comment) and followups.

Buddy frontend addition:

--- a/frontend/document/credocument.lua
+++ b/frontend/document/credocument.lua
@@ -116,6 +116,15 @@ end
 function CreDocument:engineInit()
     if not engine_initialized then
         cre = require("libs/libkoreader-cre")
+
+        -- When forking to execute any stuff in a sub-process,
+        -- as that stuff may not care about properly closing
+        -- the document, skip cre.cpp finalizer to avoid any
+        -- assertion failure.
+        require("ffi/util").addRunInSubProcessAfterForkFunc("cre_skip_teardown", function()
+            cre.setSkipTearDown(true)
+        end)
+
         -- initialize cache
         self:cacheInit()

Pinging @benoit-pierre .
(A mix of the 2 ideas: a rough "skip tear down", but with some generic way to add/remove funcs. Would be more complicated to add stuff referencing the document to do setCacheFileStale() and close() - and I didn't really like the idea of having to do that and I prefer the carefree approach.)


This change is Reviewable

Will allow frontend to disable CRE finalizers when
running code in a subprocess and explicitely exiting
quickly without any proper cleanup.
Copy link
Contributor

@benoit-pierre benoit-pierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This work for this use case, but if we generalize it, e.g. for use in the coverbrowser plugin, we would really need a pre-fork hook: you don't want to keep using the same sqlite database connection concurrently in the child process.

@poire-z
Copy link
Contributor Author

poire-z commented Apr 10, 2024

you don't want to keep using the same sqlite database connection concurrently in the child process.

Well, for this coverbrowser sqlite connection, if this was the case, I would consider it an oversight of my part. And I would consider this a "business" thing to be solved by CoverBrowser code (for its own subprocesses where we may really do use the same db connecion), and it would not have to be handled generically globally for other kinds of subprocesses (that we know that they should not do any sqlite queries on this db connection, so no harm letting it be a handle on the sqlite data file).

But it looks like we do close that DB connection:
https://github.com/koreader/koreader/blob/31c28378e7d9566969697968f6c745b14089be2a/plugins/coverbrowser.koplugin/bookinfomanager.lua#L679-L703
That is in the parent process, before launching the task in a subprocess.

I may not have worried more at the time, because from my experiences and investigation of sqlite from 10 years ago (things may have changed since?): as sqlite should work in many individual processes without anything shared in between, it can't rely on things cached in the process: it has to re-read everything from the db data files to be sure to work on not stalled data; it only locks pages when actually running a query or update, or when starting a transaction, preventing other processes from getting locks on these same pages (at the time, I think it was not even at the block/page levels, it was a generic lock on the database file). When not in a transaction, the "sqlite db connection" may just be an innofensive file handle with nothing pending on it.

@poire-z poire-z merged commit 781db25 into koreader:master Apr 10, 2024
2 checks passed
@poire-z poire-z deleted the subprocess_skip_teardown branch April 10, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants