WIP : Add KIT in process mode #7329

Darshan-upadhyay1110 · 2023-09-29T07:25:39Z

Signed-off-by: Darshan-upadhyay1110 [email protected]

Change-Id: I153bdeed50e86c9a97a60591f2644eb6b4c51f00

have a mode KITINPROCESS (limited to --enable-debug) where everything is running as a single process
global function that will know if we're in kit-in-process mode,=> Util::isKitInProcess()

vmiklos · 2023-10-02T06:19:32Z

wsd/DocumentBroker.cpp

+                        {
+                            LOG_ERR("Conversion from " << extension << " to " << newExtension << " failed (" << rc << ").");
+                            return false;
+                        }


Command used to be a separate process. It has a main function. Probably you need an else branch here, so that you invoke that main function on a thread?

vmiklos · 2023-10-02T06:20:15Z

configure.ac

@@ -192,6 +192,10 @@ AC_ARG_ENABLE([androidapp],
                              to work similarly to the iOS app, from the JavaScript and the pseudo WebSocket
                              message plumbing point of view.]))

+AC_ARG_ENABLE([kitinprocess],
+              AS_HELP_STRING([--enable-kitinprocess],


Nit: probably this is not needed, this mode could be enabled at runtime with a cmdline switch and just be enabled in debug mode. The more ifdefs we have, the more likely CI doesn't catch a build breakage.

- have a mode KITINPROCESS (limited to --enable-debug) where everything is running as a single process - global function that will know if we're in kit-in-process mode,=> Util::isKitInProcess() Signed-off-by: Darshan-upadhyay1110 <[email protected]> Change-Id: I153bdeed50e86c9a97a60591f2644eb6b4c51f00 Signed-off-by: Darshan-upadhyay1110 <[email protected]>

mmeeks

Looks like a fine start - I expect we'll want a different coolwsd process - this is most likely the easiest way to get everything linked into one binary.

Create a cool-onedoc binary or something in the Makefile.am - that links in all of the coolwsd and coolforkit source files - and then tweak / unwind the conflicts / overlaps etc.

Remove the 'spawnProcess' that starts the forkit - and instead run its 'main' or some other entry point you can add.

To avoid both 'main' functions conflicting - it's likely we'll need to split out two very small 'coolwsd-main' and 'forkit-main' files that just call an extern "C" int coolwsd_main(int argv, char **argv); type function each - since we can't have two main functions in the same linked object.

Does that help ?

mmeeks · 2023-10-03T13:59:12Z

Makefile.am

@@ -510,6 +510,12 @@ run-trace: setup-wsd
 			--o:trace[@enable]=true --o:trace.path=${builddir}/trace.txt.gz \
 			--o:trace.outgoing.record=false

+run-valgrind-kitinprocess: setup-wsd
+	@echo "Launching coolwsd under valgrind (but not forkit/coolkit, yet)"


Looks like a promising start. The idea of an unmodified coolwsd, and (perhaps) building a separate forkit which is a dynamic library we can load into the main process (or something) is possibly a good one. We want to have the forkit running in the same process as coolwsd I think.

mmeeks · 2023-10-03T14:00:38Z

kit/ForKit.cpp

@@ -383,50 +383,61 @@ static int createLibreOfficeKit(const std::string& childRoot,
    LOG_DBG("Forking a coolkit process with jailId: " << jailId << " as spare coolkit #"
                                                      << spareKitId << '.');
    const auto startForkingTime = std::chrono::steady_clock::now();
+    pid_t pid = 0;
+    if (Util::isKitInProcess()) {


Makes sense of course - we should of course guard against doing this twice - having two threads in lokit_main will be a nightmare =) can you add a LOG_ERR and perhaps a sleeping loop for that mode - I guess we have this pre-forking logic that will try to always launch a 2nd idle process when one is used so - sleeping rather than asserting makes sense for that.

Question : i am not sure about which mode you are refering here. is it the KitInProcess where the new thread has been created and we call lokit_main ?

and the LOG_ERR is also for that main function call right ?

LOK is effectively single threaded - it uses plenty of global state. You can't run two threads running lokit_main in the same process and expect it to work flawlessly. So - we should warn / block / stop this in the case that two of these are requested.

common/Util.hpp

Ashod · 2023-10-11T23:08:22Z

wsd/COOLWSD.cpp

-            if (requestURI.getPath() == FORKIT_URI)
-            {
-                if (socket->getPid() != COOLWSD::ForKitProcId)
+            if(!Util::isKitInProcess()) {


Please use clang-format to have uniform formatting everywhere. You can configure vscode to apply formatting only on edited lines.

Ashod

Thanks @Darshan-upadhyay1110. I left some comments and suggestions, as well as questions.

Ashod · 2023-10-11T23:12:07Z

common/Util.cpp

+        const char* kitInProcess = std::getenv("KIT_IN_PROCESS");
+        return (kitInProcess != nullptr && strcmp(kitInProcess, "true") == 0);


This can be static. Also, is there a reason to have it set to "true" and "false"? We can simply define the KIT_IN_PROCESS for 'true' and not define it for 'false'. And avoid this comparison (just check for != nullptr).

@Darshan-upadhyay1110 as you have separate binaries now: regular one and "single process" one, that env variable is not needed I think.
You should make this true when you link single process code probably.

Ashod · 2023-10-11T23:12:53Z

kit/ForKit.cpp

+    }
+    else {
+        pid = fork();


Please run clang-format, or apply proper formatting. This is all wrong.

thanks @Ashod i updated the formatting mistakes but there might be still some present sorry for that. i am still working on this patch so will update all formatting mistakes soon...

Ashod · 2023-10-11T23:15:14Z

kit/ForKit.cpp

+        std::thread ([childRoot, jailId, sysTemplate, loTemplate, queryVersion]
+            {
+                Util::setThreadName("kit_in_process");
+                lokit_main(childRoot, jailId, sysTemplate, loTemplate, NoCapsForKit, NoSeccomp,
+                   queryVersion, DisplayVersion, spareKitId);
+            }).detach();


Why are we detaching? Why isn't the thread owned by coolwsd?

Can you share the design of this whole PR please?

Ashod · 2023-10-11T23:16:53Z

kit/ForKit.cpp

-#ifndef KIT_IN_PROCESS
-        UnitKit::get().launchedKit(pid);
-#endif
+        if (!Util::isKitInProcess()) {


Please do not use javascript formatting in C++ code. Apply clang-format if it's too much work.

Ashod · 2023-10-11T23:29:02Z

wsd/debug-main.cpp

+    // Wait for the thread to finish and get the result using a future.
+    std::future<int> resultFuture = resultPromise.get_future();
+    int result = resultFuture.get();


The proper way of waiting for a thread to finish is to join the thread.

There is no good reason to detach the thread and use a promise/future.

int result = 0; std::thread worker = std::thread ([&] { Util::setThreadName("debug main kit_in_process"); result = forkit_main(args.size(), argv); }); worker.join(); return result;

Ashod · 2023-10-11T23:29:41Z

wsd/COOLWSD-main.cpp

@@ -0,0 +1,40 @@
+/*


Not clear to me why we need all these new files. Is there an explanation on the design somewhere?

@Ashod comment above will explain: #7329 (review)

Ashod · 2023-10-11T23:32:34Z

kit/ForKit.cpp

@@ -366,6 +361,8 @@ static void cleanupChildren()
    }
 }

+static bool lokitProcessIsRunning = false;


Please add comments so it is clear what the various variables/functions do. It's not clear what this is supposed to represent and how it's supposed to work.

i have added in some places but as i may change code bits as per task progress further so probably add all commets once i got working build , thanks :)

Ashod · 2023-10-11T23:33:55Z

wsd/COOLWSD.cpp

+    if (!Util::isKitInProcess()) {
+        SigUtil::setFatalSignals("wsd " COOLWSD_VERSION " " COOLWSD_VERSION_HASH);
+    }


Why? Why remove the fatal signal in 'kit in process' mode?

ohh yes, sorry we only need to remove fatal signal for Forkit and Kit not for COOLWSD ( pushing that change...)

- debug bin program for single Kit in process - split out two very small 'coolwsd-main' and 'forkit-main' - add debug-main for kit in process Signed-off-by: Darshan-upadhyay1110 <[email protected]> Change-Id: Iff8d829feead221b1f08c53fb1dbfb4241757601

Signed-off-by: Darshan-upadhyay1110 <[email protected]> Change-Id: I6bf029df1f9b7fe395ecfdf6144a1869efcf0833

Signed-off-by: Jaume Pujantell <[email protected]> Change-Id: Ib08067e83ab08ce5bc525f069357218b3d3614db

JaumePujantell · 2023-11-03T17:31:30Z

Push my current local state (not working) so @caolanm can see it.

mmeeks · 2024-02-27T13:16:16Z

Eventually we merged this in another pull request; thanks Darshan =)

Darshan-upadhyay1110 requested a review from eszkadev September 29, 2023 07:25

Darshan-upadhyay1110 force-pushed the private/darshan/Kit-in-process branch 2 times, most recently from d0cefb5 to 54ad034 Compare September 29, 2023 14:50

vmiklos reviewed Oct 2, 2023

View reviewed changes

Darshan-upadhyay1110 force-pushed the private/darshan/Kit-in-process branch 8 times, most recently from ad8bec1 to 9817c36 Compare October 3, 2023 12:10

mmeeks reviewed Oct 3, 2023

View reviewed changes

hfiguiere reviewed Oct 3, 2023

View reviewed changes

common/Util.hpp Show resolved Hide resolved

Darshan-upadhyay1110 force-pushed the private/darshan/Kit-in-process branch 10 times, most recently from 483dc72 to 7c63f31 Compare October 10, 2023 16:04

Ashod reviewed Oct 11, 2023

View reviewed changes

Darshan-upadhyay1110 force-pushed the private/darshan/Kit-in-process branch from 7c63f31 to 1116bb9 Compare October 12, 2023 16:50

Add ForKitLibreOfficeKit for debug binary

6e3a541

Signed-off-by: Darshan-upadhyay1110 <[email protected]> Change-Id: I6bf029df1f9b7fe395ecfdf6144a1869efcf0833

Some more WIP

bf01942

Signed-off-by: Jaume Pujantell <[email protected]> Change-Id: Ib08067e83ab08ce5bc525f069357218b3d3614db

mmeeks closed this Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP : Add KIT in process mode #7329

WIP : Add KIT in process mode #7329

Darshan-upadhyay1110 commented Sep 29, 2023

vmiklos Oct 2, 2023

vmiklos Oct 2, 2023

mmeeks left a comment

mmeeks Oct 3, 2023

mmeeks Oct 3, 2023

Darshan-upadhyay1110 Oct 3, 2023

mmeeks Oct 3, 2023

Ashod Oct 11, 2023

Ashod left a comment

Ashod Oct 11, 2023

eszkadev Oct 12, 2023

Ashod Oct 11, 2023

Darshan-upadhyay1110 Oct 12, 2023 •

edited

Loading

Ashod Oct 11, 2023

Ashod Oct 11, 2023

Ashod Oct 11, 2023

Ashod Oct 11, 2023

eszkadev Oct 12, 2023

Ashod Oct 11, 2023

Darshan-upadhyay1110 Oct 12, 2023

Ashod Oct 11, 2023

Darshan-upadhyay1110 Oct 12, 2023

JaumePujantell commented Nov 3, 2023

mmeeks commented Feb 27, 2024

		const char* kitInProcess = std::getenv("KIT_IN_PROCESS");
		return (kitInProcess != nullptr && strcmp(kitInProcess, "true") == 0);

WIP : Add KIT in process mode #7329

WIP : Add KIT in process mode #7329

Conversation

Darshan-upadhyay1110 commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmeeks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ashod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darshan-upadhyay1110 Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaumePujantell commented Nov 3, 2023

mmeeks commented Feb 27, 2024

Darshan-upadhyay1110 Oct 12, 2023 •

edited

Loading