Bug 26791 - Build Mojolicious controller replacement for export.pl
Summary: Build Mojolicious controller replacement for export.pl
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
: 26792 (view as bug list)
Depends on: 27031
  Show dependency treegraph
Reported: 2020-10-22 23:02 UTC by David Cook
Modified: 2023-11-07 02:37 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:

Bug 26791: Replace export.pl with export Mojolicious controller (20.19 KB, patch)
2020-11-16 11:56 UTC, David Cook
Details | Diff | Splinter Review
Bug 26791: Use Mojolicious record exporter when using ExportCircHistory (8.52 KB, patch)
2020-12-22 09:26 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2020-10-22 23:02:22 UTC
As per my notes on Bug 8437, starting at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8437#c33, any Koha .pl script that needs to stream a large response to a client web browser is never going to work, so long as we're using Plack::App::CGIBin to create a PSGI handler. 

While I tried to create my own streaming version of Plack::App::WrapCGI, I found a third-party module to be buggy, and that writing my own would be prohibitive in terms of time and energy. It's also architectural glue code that we really might not want to maintain long-term.

So I've concluded that an alternative would be to write a Mojolicious controller to replace export.pl. While CGI Koha can still use export.pl, when we're using Starman/Plack, we can redirect to the appropriate Mojolicious endpoint.
Comment 1 David Cook 2020-10-23 02:39:33 UTC Comment hidden (obsolete)
Comment 2 David Cook 2020-10-23 03:04:26 UTC Comment hidden (obsolete)
Comment 3 David Cook 2020-10-23 03:51:24 UTC Comment hidden (obsolete)
Comment 4 David Cook 2020-10-23 04:07:47 UTC Comment hidden (obsolete)
Comment 5 David Cook 2020-10-23 04:39:44 UTC Comment hidden (obsolete)
Comment 6 David Cook 2020-10-23 06:30:21 UTC Comment hidden (obsolete)
Comment 7 David Cook 2020-10-23 06:33:29 UTC Comment hidden (obsolete)
Comment 8 David Cook 2020-10-23 07:37:01 UTC Comment hidden (obsolete)
Comment 9 David Cook 2020-10-23 07:42:34 UTC Comment hidden (obsolete)
Comment 10 David Cook 2020-10-23 09:04:42 UTC
So it seems that $c->write and $c->write_chunk just put the content in a buffer rather than actually writing to the socket. (It's too bad they're not called $c->buffer and $c->buffer_chunk >_>.) 

The buffer doesn't get emptied until "generate_body_chunk" is called... and a stacktrace suggests in this case it's due to the drain event being fired. 

Then like Node.js, you have recursive callbacks where each callback is called by an event being fired and that callback processes 1 result and sets up the next callback.

It's complicated, but it should be performant. If I wrote a native PSGI handler, it would be blocking, and if you only have 2 Starman workers, that means 1 whole worker would be occupied with streaming the results of 1 large export...

I've written to the Mojolicious listserv to see if there is an easier way, but I think that's probably how it's going to go.

Any more investigation will have to wait until a day where I don't stay at work until 8pm...
Comment 11 David Cook 2020-10-26 01:07:46 UTC
Sebastian Riedel, the founder of Mojolicious, mentioned that the drain callback is the only way to do streaming in Mojolicious, since it relies on an event loop and non-blocking I/O, which makes sense. 

You write some data to the buffer, enqueue an event, the event loop processes the queue (which could include other events like a new web request, some I/O, or whatever), and then you repeat the process over and over until you're out of data. This asynchronous style is not the most intuitive to write, but it does make for a much more powerful and robust web server.

Sebastian also mentioned promises and async/await. The async/await (https://docs.mojolicious.org/Mojolicious/Guides/Cookbook#async-await) looks very user-friendly, but it's an *experimental* feature and requires at least Perl 5.16+ I think and preferrably 5.24+. (Note Debian Stretch comes with perl 5.24.)

Alternatively, I could look at wrapping it in a Mojo::Promise. That might help in terms of readability/maintainability at least.
Comment 12 David Cook 2020-10-30 02:28:52 UTC Comment hidden (obsolete)
Comment 13 David Cook 2020-11-16 11:20:19 UTC
Making significant progress.

koha-run-backups --days 2 --output /var/spool/koha
ls /var/spool/koha/kohadev/
kohadev-2020-11-16.sql.gz  kohadev-2020-11-16.tar.gz

I can't download kohadev-2020-11-16.tar.gz via the web interface because of a permission issue that seems to already exist in Koha.

However, I was able to download kohadev-2020-11-16.sql.gz in 22-49ms using the Mojolicious controller (vs 1.46-1.97 seconds with export.pl).
Comment 14 Victor Grousset/tuxayo 2020-11-16 11:35:06 UTC
Impressive :D
Comment 15 David Cook 2020-11-16 11:53:43 UTC Comment hidden (obsolete)
Comment 16 David Cook 2020-11-16 11:56:17 UTC
Created attachment 113660 [details] [review]
Bug 26791: Replace export.pl with export Mojolicious controller

Comment 17 David Cook 2020-11-16 12:01:42 UTC
I am still working on this, but I'm sharing so that people can review, provide feedback, or whatever.

Test plan:
1. cp debian/templates/plack.psgi /etc/koha/sites/kohadev/plack.psgi
2. koha-plack --restart kohadev
3. Go to http://localhost:8081/cgi-bin/koha/staff/tools/export
(Alternatively, navigate to http://localhost:8081/cgi-bin/koha/tools/tools-home.pl and click "Export data"
4. Visually compare with http://localhost:8081/cgi-bin/koha/tools/export.pl
(Note that the top nav is slightly different because I am being very opinionated about how I populate $template variables to reduce future technical debt.)

Other test plans:
0. First, follow the above test plan
1. koha-run-backups --days 2 --output /var/spool/koha
2. vi /etc/koha/sites/kohadev/koha-conf.xml
3. Enable backup_db_via_tools and backup_conf_via_tools
4. echo 'flush_all' | nc -q 1 memcached 11211
5. koha-plack --restart kohadev
6. Go to http://localhost:8081/cgi-bin/koha/staff/tools/export
7. Click "Export database"
8. Choose a file and click "Download database"
9. Note that it downloads quickly and correctly
(Bonus points if you try downloading a very large database. I have so far only tried with small databases, but it should work with large ones too.)
Comment 18 David Cook 2020-11-16 12:13:25 UTC
Another test plan:
0. Follow the first test plan
1. koha-plack --disable kohadev
2. service apache2 restart
3. Go to http://localhost:8081/cgi-bin/koha/staff/tools/export
4. Click "Export database", choose a file, and click "Download database"
5. Note that the database downloads successfully (even in CGI mode!)
Comment 19 David Cook 2020-11-16 23:47:06 UTC
I just did a test with a 41MB Koha database and it took 17ms to download from http://localhost:8081/cgi-bin/koha/staff/tools/export. I've verified the file and it's complete. 

Using http://localhost:8081/cgi-bin/koha/tools/export.pl it took 1.4s.
Comment 20 David Cook 2020-12-01 22:43:24 UTC Comment hidden (obsolete)
Comment 21 David Cook 2020-12-22 06:20:27 UTC Comment hidden (obsolete)
Comment 22 David Cook 2020-12-22 09:26:02 UTC
Created attachment 114587 [details] [review]
Bug 26791: Use Mojolicious record exporter when using ExportCircHistory

This patch uses the Mojolicious record exporter to export records from
the Circulation module.

Test plan:
1. Change system preference "ExportCircHistory" to "Show"
2. Checkout items 39999000004571 and 39999000001310 to the "koha"
3. Select checkouts from list and check the "Export" checkboxes
4. Click "Export" at the bottom of the page
5. Note that valid ISO 2709 MARC is generated in the export
Comment 23 David Cook 2020-12-22 09:38:16 UTC
I still have more work to do, but the core is there. It's really now just about building out the controller, hopefully moving code into modules, and adding unit tests for those modules.

Actually, exporting as CSV is going to be annoying, but I have some ideas.

I'm hoping to look at this more over the holidays.
Comment 24 David Cook 2020-12-24 00:45:27 UTC
(In reply to David Cook from comment #23)
> I'm hoping to look at this more over the holidays.

Next steps:
1. Finish Perl coding
  a. Finish controller code
  b. Move code out of controller and into modules
  c. Possibly refactor or add new CSV export code...
2. Write unit tests for modules
3. Final E2E testing
Comment 25 David Cook 2021-05-12 00:07:35 UTC
Unfortunately, I haven't had any time/energy to work on this, although it's still something I want to do eventually.
Comment 26 David Cook 2023-11-07 02:37:57 UTC
The patches are outdated and realistically we're not moving in this direction anyway.

It's probably more realistic to handle this as a background task. It would allow the web app to be more available. The only downside is that it will take up disk space.