Right now, it is not possible to create custom Koha::Object(s) for a plugin without breaking things. For example we did this: BEGIN { my $path = Module::Metadata->find_module_by_name(__PACKAGE__); $path =~ s!\.pm$!/lib!; unshift @INC, $path; require Koha::CurbsidePickupIssues; require Koha::CurbsidePickupPolicies; require Koha::CurbsidePickups; require Koha::Schema::Result::CurbsidePickup; require Koha::Schema::Result::CurbsidePickupIssue; require Koha::Schema::Result::CurbsidePickupPolicy; # register the additional schema classes Koha::Schema->register_class(CurbsidePickup => 'Koha::Schema::Result::CurbsidePickup'); Koha::Schema->register_class(CurbsidePickupPolicy => 'Koha::Schema::Result::CurbsidePickupPolicy'); Koha::Schema->register_class(CurbsidePickupIssue => 'Koha::Schema::Result::CurbsidePickupIssue'); # ... and force a refresh of the database handle so that it includes # the new classes Koha::Database->schema({ new => 1 }); } In the curbside pickups plugin before it was integrated. The problem we've found is that calling Koha::Database->schema({ new => 1 }); resets the database connection and breaks any transactions you were in when the plugin was loaded! We should be able to search for and load any schema files found in plugin directories to make this all unnecessary.
Forgot to mention, we probably need to do that search and load in Koha::Schema
So, I played around here a bit - my first thought was that we could simply avoid the reconnect if the objects were already loaded, so I updated a plugin like so: $schema = Koha::Database->schema({ new => 1 }) unless $schema->source_registrations->{KohaPluginComBywatersolutionsContractsContract}; With some warns I found that this was executed when plack was restarted, so the object are loaded in plack from the start, that's good. But I found that imports still failed and caused reconnect - that's bad. What I eventually realized was that we are forking our background jobs - and these get a new DB connection, without the new objects. The dirty fix that worked was adding to the worker: Koha::Plugins->get_enabled_plugins; After spawning the process, but before beginning the job. And that fixes it. I am not sure where we load the plugins when we start plack, but if we did the same thing when launching a background worker process - or built it in to the process method of BackgroundJob To move the logic out of the plugin we could add a 'plugin_classes' method and have Koha::Schema check for if the objects are loaded - but I think maybe just loading plugins for the background tasks at spawn is a good start?
That sounds like a reasonable plan to me!
Created attachment 168861 [details] [review] Bug 36736: Load plugins at the start of background job processing This patch adds a call to get_enabled_plugins before processing background jobs to ensure that all plugin hooks are loaded and cached To test: 1 - Install a plugin that adds new objects e.g. the Contracts plugin https://github.com/bywatersolutions/fs-koha-plugin-contracts or the KOha Advent plugin: https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin 2 - Restart all 3 - Tail all your logs 4 - Stage and import a file containing items 5 - Note in the logs DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction 6 - Apply this patch 7 - Restart all 8 - Stage and import again 9 - Success!
Sounds interesting. Could you walk me/us through this a bit more?
(In reply to David Cook from comment #5) > Sounds interesting. Could you walk me/us through this a bit more? The idea is really to make plugin code a bit simpler, easier to read, more consistent with Koha core, and leave open a path for future integration. Adding new objects via the plugin lets plugins use KohaTables and other helper code as well. Adding objects was featured in the Koha Advent plugin, but it seems the background jobs broke this trick This patch just ensures the plugins are loaded, and hence all functions and objects available, for our background processes
(In reply to Nick Clemens (kidclamp) from comment #6) > Adding new objects via the plugin lets plugins use KohaTables and other > helper code as well. I certainly like the sound of that! > Adding objects was featured in the Koha Advent plugin, but it seems the > background jobs broke this trick > > This patch just ensures the plugins are loaded, and hence all functions and > objects available, for our background processes So I was hoping you'd be able to explain it easier than I'd be able to puzzle it out by trying it myself, but I think it's one of those things where a person just needs to try it out. Trying the test plan, and I see the errors in /var/log/koha/kohadev/worker-output.log I'm a bit confused though... I installed the plugin, restarted everything, and we're using a plugin that isn't used in the MARC import background job. So I feel like we're not really addressing the root problem here. Actually... looking at the log /var/log/koha/kohadev/worker-output.log again I see this: Argument "{VERSION}" isn't numeric in int at /kohadevbox/koha/Koha/Plugins/Base.pm line 339. Argument "{VERSION}" isn't numeric in int at /kohadevbox/koha/Koha/Plugins/Base.pm line 339. I'm going to investigate that a bit further...
Ah, so, basically the background job looks for plugins with an 'after_biblio_action' hook. To find them we end up loading all the plugins.modules. This is what causes the begin block to fire again. If we call GetPlugins before this check, everything is cached and we don't reload the modules, so we don't trigger the new dbic
Okie dokie... So Koha/BackgroundJob.pm looks up any plugins by querying the database for the method "background_tasks". Excellent. That means only plugins that have the background_tasks method get loaded by the background worker when it's getting set up (prior to forking). 448 my @plugins = Koha::Plugins->new()->GetPlugins( { method => 'background_tasks', } ); -- Unfortunately... during the work of Koha/BackgroundJob/MARCImportCommitBatch.pm, we call C4::Biblio::ModBiblioMarc, which runs the following code: 2914 Koha::Plugins->call( 2915 'before_biblio_action', 2916 { action => 'save', payload => { biblio_id => $biblionumber, record => $record } } 2917 ); The "call" function loads every installed plugin and then checks if the Perl object "can" do a particular method. That causes the problem that Kyle and Nick have noted. -- We could use the patch attached here, but I'm thinking that we might want to first fix the Koha::Plugins->call() so that it uses the database lookup for the method, caches it, and goes from there. That should hopefully fix this problem *plus* improve the efficiency of Koha::Plugins->call(). I wonder if there's already a bug for that in fact...
(In reply to Nick Clemens (kidclamp) from comment #8) > Ah, so, basically the background job looks for plugins with an > 'after_biblio_action' hook. To find them we end up loading all the > plugins.modules. This is what causes the begin block to fire again. If we > call GetPlugins before this check, everything is cached and we don't reload > the modules, so we don't trigger the new dbic Something like that. I guess I haven't worked out the exact cause. Just that it's C4::Biblio::ModBiblioMarc loading all the modules, and that's triggering that BEGIN block on the Koha Advent plugin, which is doing that connection reset within the BatchMarcCommit transaction.
(In reply to David Cook from comment #9) > We could use the patch attached here, but I'm thinking that we might want to > first fix the Koha::Plugins->call() so that it uses the database lookup for > the method, caches it, and goes from there. It looks like I talked to Julian about this years ago on bug 29672. I thought there was a newer bug... -- I was concerned about RAM usage here, but it looks like you've added the "Koha::Plugins->get_enabled_plugins();" after the background_jobs_worker.pl forks, so that's good. It would be more efficient to update Koha::Plugins->call(), but that would be more work. This is a practical solution... -- On a side note, for Koha::Plugin::FancyPlugin, does the Koha::Schema->register_class and Koha::Database->schema({ new => 1 }) have to be in the BEGIN block? Could you wrap it with a check for a plack environmental variable?
Created attachment 169389 [details] [review] Bug 36736: Load plugins at the start of background job processing This patch adds a call to get_enabled_plugins before processing background jobs to ensure that all plugin hooks are loaded and cached To test: 1 - Install a plugin that adds new objects e.g. the Contracts plugin https://github.com/bywatersolutions/fs-koha-plugin-contracts or the KOha Advent plugin: https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin 2 - Restart all 3 - Tail all your logs 4 - Stage and import a file containing items 5 - Note in the logs DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction 6 - Apply this patch 7 - Restart all 8 - Stage and import again 9 - Success! Signed-off-by: David Cook <dcook@prosentient.com.au>
If someone else adds a sign off, I think I could convert this to a Passed QA.
Created attachment 169410 [details] [review] Bug 36736: Load plugins at the start of background job processing This patch adds a call to get_enabled_plugins before processing background jobs to ensure that all plugin hooks are loaded and cached To test: 1 - Install a plugin that adds new objects e.g. the Contracts plugin https://github.com/bywatersolutions/fs-koha-plugin-contracts or the Koha Advent plugin: https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin 2 - Restart all 3 - Tail all your logs 4 - Stage and import a file containing items 5 - Note in the logs DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction 6 - Apply this patch 7 - Restart all 8 - Stage and import again 9 - Success! Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm writing tests for this. Hold on.
Created attachment 169440 [details] [review] Bug 36736: Tests [POC]
Hi, my work day is ending and I wanted to share what I've got so far. I haven't managed to reproduce the error so far. I've added: * A schema file * Koha::Object-based classes for that schema * Tweaked the Test plugin so it has the usual ->register call * A plugin background job that (for now) just prints the schema it finds. I expected it to not have the schemas defined, but they are. I hope I missed something obvious and someone beats me on finishing the tests before I wake up :-D
(In reply to Tomás Cohen Arazi from comment #17) > Hi, my work day is ending and I wanted to share what I've got so far. I > haven't managed to reproduce the error so far. > > I've added: > * A schema file > * Koha::Object-based classes for that schema > * Tweaked the Test plugin so it has the usual ->register call > * A plugin background job that (for now) just prints the schema it finds. > > I expected it to not have the schemas defined, but they are. > > I hope I missed something obvious and someone beats me on finishing the > tests before I wake up :-D You won't be able to reproduce the problem in "t/db_dependent/Koha/Plugins/Plugins.t", because Koha::Plugin::Test is already loaded in the BEGIN block of this test file. We'll need to have a separate test file just for this test. We'll want to add a Koha::Plugins->call() within a database transaction into Koha::BackgroundJob::TestBackgroundJob->process(), because that's what triggers the bug. Koha::Plugins->call() loads all enabled plugins, which will then reset the connection, and since we're in a database transaction, then we'll get a lock issue I believe.
Let's see if I can really quickly bang this out...
(In reply to David Cook from comment #19) > Let's see if I can really quickly bang this out... Interrupted by too many meetings. Hoping to sit down and look at this later today.
I've had a go at writing the test, but I've actually made it so that even with the patch, the problem will still occur. I think because we wrap all our tests with transactions it makes the database connection stuff problematic. But then again... I'm having all kinds of weird database issues in this test. Like I'm inserting data before the transactions... and I still can't get the following to work: my @plugins = Koha::Plugins->new()->GetPlugins( { method => 'background_tasks', } ); But I've wasted way too much time on this already... I don't think I can work on this anymore.
Btw another thing I noticed with the current attached test... in order to utilize Nick's change, you need to use create the background job via Koha::BackgroundJob and call its process() method, which then will invoke the background job type listed in its "type" attribute. Like Julian says on bug 29672, maybe we should be relying on a standardized bootstrap step instead. In any case, I think there's a lot of pain ahead with this in any case...
It seems I forgot to add my last attempt yesterday: ``` index 460b710a4e5..58ffc9aadac 100755 --- a/t/db_dependent/Koha/Plugins/Plugins.t +++ b/t/db_dependent/Koha/Plugins/Plugins.t @@ -322,9 +322,13 @@ subtest 'Koha::Plugin::Test' => sub { plan tests => 1; - use DDP; - my $job = Koha::BackgroundJob::TestBackgroundJob->new; - $job->process; + # Launch the sleep script + my $pid = fork(); + if ( $pid == 0 ) { + use DDP; + my $job = Koha::BackgroundJob::TestBackgroundJob->new; + $job->process; + } ``` which didn't work either :-P
Created attachment 169508 [details] [review] Bug 36736: Tests [POC]
So it now explodes with `Exception 'Koha::Exception' thrown 'test_background_job is not a valid job_type'` meaning the plugins didn't get initialized?
(In reply to Tomás Cohen Arazi from comment #25) > So it now explodes with > > `Exception 'Koha::Exception' thrown 'test_background_job is not a valid > job_type'` > > meaning the plugins didn't get initialized? Yeah, that was the same error I ended up dealing with. I didn't have time to puzzle it out. (In reply to David Cook from comment #21) > But I've wasted way too much time on this already... I don't think I can > work on this anymore. Apologies if this sounded harsh. I was just feeling really frustrated after struggling for a long time with the unit test. I really can't rationalize spending more time on this one, when I've got a lot of other things that I know I can make progress instead of spinning my wheels :/.
Still think the change overall is interesting though! I do think that some of the issues with the unit tests does suggest that this current strategy might not be enough, and that maybe we should try to plan/design this more carefully...
I'm tempted to move the tests to a new bug.. whilst it would be lovely to cover it I'm keen to get the fix into core sooner rather than later.
(In reply to Martin Renvoize from comment #28) > I'm tempted to move the tests to a new bug.. whilst it would be lovely to > cover it I'm keen to get the fix into core sooner rather than later. +1
Created attachment 169555 [details] [review] Bug 36736: Load plugins at the start of background job processing This patch adds a call to get_enabled_plugins before processing background jobs to ensure that all plugin hooks are loaded and cached To test: 1 - Install a plugin that adds new objects e.g. the Contracts plugin https://github.com/bywatersolutions/fs-koha-plugin-contracts or the Koha Advent plugin: https://gitlab.com/koha-community/koha-advent/koha-plugin-fancyplugin 2 - Restart all 3 - Tail all your logs 4 - Stage and import a file containing items 5 - Note in the logs DBI Exception: DBD::mysql::st execute failed: Lock wait timeout exceeded; try restarting transaction 6 - Apply this patch 7 - Restart all 8 - Stage and import again 9 - Success! Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Is that really major or not more an enh?
Hm, the title of the bug and the content of the patch don't seem to match up. Maybe we should update the bug description?
Pushed for 24.11! Well done everyone, thank you!
I'm curious how this would interact with bug 36721, as I preload Koha::Schema there into the Starman master process.
(In reply to David Cook from comment #35) > I'm curious how this would interact with bug 36721, as I preload > Koha::Schema there into the Starman master process. My instinct is that your patch won't interact with this one, as the only module you load from the Koha namespace is Koha::Schema which itself doesn't anything else in the Koha or C4 namespaces. I suppose that begs the question of "should we include Koha/C4 packages in Preload.pm" but that's more of a question to be answered on that bug :)
(In reply to Kyle M Hall from comment #36) > (In reply to David Cook from comment #35) > > I'm curious how this would interact with bug 36721, as I preload > > Koha::Schema there into the Starman master process. > > My instinct is that your patch won't interact with this one, as the only > module you load from the Koha namespace is Koha::Schema which itself doesn't > anything else in the Koha or C4 namespaces. I suppose that begs the question > of "should we include Koha/C4 packages in Preload.pm" but that's more of a > question to be answered on that bug :) Yeah, I don't think it will conflict either. I figure your plugin just adds on top of the Koha::Schema::ResultSet classes, so it should be fine...
looks like we are still waiting for tests here.