Bug 31744

Summary: Inventory tool can take a long time and never complete
Product: Koha Reporter: Liz Rea <wizzyrea>
Component: Staff interfaceAssignee: David Cook <dcook>
Status: In Discussion --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: chughesman, dcook, emily.lamancusa, gmcharlt, jonathan.druart, m.de.rooy, mathsabypro, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33989
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33991
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12733
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31216
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Attachments: Bug 31744: Run inventory.pl as CGI with keep alives to prevent timeouts
Bug 31744: Run inventory.pl as CGI with keep alives to prevent timeouts

Description Liz Rea 2022-10-12 16:22:10 UTC
There seems to be a problem with scaling for the inventory tool. More and more often now with bigger libraries we are running into libraries putting too many barcodes into the tool, and timing out the tool, or not getting feedback on the job progress and clicking back and trying again (and again. And again.), eventually causing a denial of service as they tie up all of their plack workers with inventory jobs.

Potentially we could put the processing part into the background jobs or work on the performance here some other way so that larger batches could be accommodated? The servers weren't underpowered for this - the libraries were putting in collection-size-appropriate numbers of barcodes.

Open to any kind of solution here that would help make this tool more usable for libraries.

Cheers,
Liz
Comment 1 David Cook 2023-06-09 05:38:47 UTC
This is on my mind as well. I have a few libraries that have corporate HTTP forward proxies that time out after 60 seconds, which makes the inventory tool very difficult if not impossible to use.
Comment 2 David Cook 2023-06-09 05:53:22 UTC
This one seems a bit intimidating because there are different modes for the inventory and no one uses the inventory tool that much I think.

When you provide barcodes, I think that you end up with an output that is either output to screen or to a CSV output. (This type of inventory can be evaluated against only itself or against "results" which are determined based on the other filters?)

But if you don't provide a barcode list, then there is an interactive mode based off the "results" of the filters... and this list fetches all matches into a DataTable.

I suppose both modes would be amenable to background processing. 

In terms of database schema, I think we'd need to store the items in the "results" with a column for problems? Maybe a "batch" table would have some reporting columns for barcodes read and items modified.

That's my interpretation at a glance. There might be other scenarios to take into account...
Comment 3 David Cook 2023-06-09 05:56:01 UTC
(In reply to David Cook from comment #2)
> That's my interpretation at a glance. There might be other scenarios to take
> into account...

Looks like there can be other results like "Barcode not found". 

Not sure how we'd handle those kinds of things in a translation-friendly way... but maybe I just need to hook into the code and see.
Comment 4 David Cook 2023-06-13 03:55:53 UTC
(In reply to Liz Rea from comment #0)
> There seems to be a problem with scaling for the inventory tool. More and
> more often now with bigger libraries we are running into libraries putting
> too many barcodes into the tool, and timing out the tool, or not getting
> feedback on the job progress and clicking back and trying again (and again.
> And again.)

I've found a flaw in the inventory tool and fixing that should speed things up a fair bit, but probably still not enough for bigger libraries.

--

I've also got an idea for a workaround that could get the tool to work for larger barcode sets which should prevent timeouts from occurring... but I don't have a plan for avoiding them not getting feedback on the job progress and clicking back and trying again and again...

(Basically, excluding inventory.pl from Plack processing, and then sending keep alive messages in the form of spaces or HTML comments. The reason for excluding from Plack is that Plack::App::CGIBin buffers the complete response before sending it back to the client (and any proxies in between).)

--

I think changing the inventory to use background processing is still a necessary step, but the above are some changes that would get the tool working again in a short amount of time.
Comment 5 David Cook 2023-06-13 05:01:17 UTC Comment hidden (obsolete)
Comment 6 David Cook 2023-06-13 05:01:51 UTC Comment hidden (obsolete)
Comment 7 David Cook 2023-06-13 05:04:33 UTC Comment hidden (obsolete)
Comment 8 David Cook 2023-06-13 06:08:36 UTC Comment hidden (obsolete)
Comment 9 David Cook 2023-06-14 01:21:59 UTC Comment hidden (obsolete)
Comment 10 David Cook 2023-06-14 01:23:16 UTC Comment hidden (obsolete)
Comment 11 David Cook 2023-06-14 02:06:17 UTC Comment hidden (obsolete)
Comment 12 David Cook 2023-06-14 07:13:24 UTC Comment hidden (obsolete)
Comment 13 David Cook 2023-06-14 23:34:40 UTC Comment hidden (obsolete)
Comment 14 David Cook 2023-06-15 01:19:02 UTC
In the end, the trick was to override AddOutputFilterByType just for inventory.pl. 

So overall:
1. Don't proxy inventory.pl to Starman/Plack
2. Override AddOutputFilterByType so the DEFLATE compression output filter isn't added
3. At the start of the inventory.pl processing output the HTTP headers and then roughly every 5 seconds print out a NULL character to keep alive the HTTP connection until the HTML template/CSV response is ready to be printed out.

--
Comment 15 David Cook 2023-06-15 01:41:22 UTC
Created attachment 152370 [details] [review]
Bug 31744: Run inventory.pl as CGI with keep alives to prevent timeouts

This patches causes inventory.pl to run as CGI instead of PSGI,
since Plack::App::CGIBin buffers the whole response before sending any
data to the Apache reverse proxy.

This patch also causes inventory.pl to quickly output HTTP headers,
and print NULL characters roughly every 5 seconds until the HTML
or CSV data is ready to be printed. This prevents HTTP forward and
reverse proxies from timing out waiting for data from the inventory
script, which can be very slow.

In the future, we will want to move inventory.pl to use background
jobs, but this change makes inventory.pl usable for staff users stuck
behind HTTP forward proxies with short timeouts.

Test plan:
0. Apply patch
1. cp debian/templates/apache-shared-intranet-plack.conf
/etc/koha/apache-shared-intranet-plack.conf
2. cp debian/templates/apache-shared.conf
/etc/koha/apache-shared.conf
3. service apache2 restart

4. Go to create a SQL report from SQL
/cgi-bin/koha/reports/guided_reports.pl?phase=Create%20report%20from%20SQL
5. Save a report with the following SQL:
SELECT barcode FROM items where barcode <> '';
6. Run the report
7. Download as CSV
8. Edit the CSV and remove the "barcode" heading

9. Open F12 dev tools in browser and open the "Network" tab
10. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
11. Note that mainpage.pl has a "Content-Encoding: gzip" header
12. Go to http://localhost:8081/cgi-bin/koha/tools/inventory.pl
13. Note that inventory.pl does not have a "Content-Encoding: gzip"
header, while the .js and .css files do have this header.

14. Click "Choose file" and choose your barcode CSV file
15. Uncheck "Compare barcodes list to results"
16. Check "Export to CSV file"
17. Click "Submit"

18. Note that the download initially only increases in size by 1B at a time
19. Open inventory.csv with a text editor that can show symbols like Notepad++
20. Note approximately 7 or 8 NULL characters at start of file
21. Note that CSV loads without problems in Excel or LibreOffice (ie the NULL
characters are ignored)

22. Return to your browser and uncheck "Export to CSV file"
23. Click "Submit"
24. On the "Response" tab, note approximately 7 or 8 NULL characters at start
of response

--

25. For bonus points, set up a reverse proxy in front of inventory.pl
with a proxy timeout of 10 seconds and run an inventory job. Note that
the proxy times out without the patch, and does *not* time out with the patch.
Comment 16 David Cook 2023-06-15 01:50:20 UTC
So far I've only tested this in koha-testing-docker (using a ProxyPass to inventory.pl from a custom endpoint - needed to update inventory.tt to point to that custom endpoint).

It works as expected, but I typically run Koha behind yet another reverse proxy, so I'm curious how that will go. I suspect that I'll need to disable compression on that proxy for inventory.pl. 

(Note that compression isn't enabled for CSV exports from Koha by default anyway, so it's not a huge drama in terms of the status quo...)
Comment 17 David Cook 2023-06-15 03:44:33 UTC
(In reply to David Cook from comment #16)
> So far I've only tested this in koha-testing-docker (using a ProxyPass to
> inventory.pl from a custom endpoint - needed to update inventory.tt to point
> to that custom endpoint).
> 
> It works as expected, but I typically run Koha behind yet another reverse
> proxy, so I'm curious how that will go. I suspect that I'll need to disable
> compression on that proxy for inventory.pl. 
> 
> (Note that compression isn't enabled for CSV exports from Koha by default
> anyway, so it's not a huge drama in terms of the status quo...)

I've tried this on a more real-world setup, and it works well even when sitting behind another Apache reverse proxy that has compression enabled. It doesn't seem to buffer. It just starts streaming the data. 

(It does need the compression disabled for inventory.pl on the Apache sitting in front of Starman though.)
Comment 18 David Cook 2023-06-15 05:49:43 UTC
Real world library confirmed that this change resolves the timeout problem when using their corporate HTTP forward proxy which has a 60 second timeout threshold.
Comment 19 Emily Lamancusa (emlam) 2023-08-04 20:05:39 UTC
Created attachment 154297 [details] [review]
Bug 31744: Run inventory.pl as CGI with keep alives to prevent timeouts

This patches causes inventory.pl to run as CGI instead of PSGI,
since Plack::App::CGIBin buffers the whole response before sending any
data to the Apache reverse proxy.

This patch also causes inventory.pl to quickly output HTTP headers,
and print NULL characters roughly every 5 seconds until the HTML
or CSV data is ready to be printed. This prevents HTTP forward and
reverse proxies from timing out waiting for data from the inventory
script, which can be very slow.

In the future, we will want to move inventory.pl to use background
jobs, but this change makes inventory.pl usable for staff users stuck
behind HTTP forward proxies with short timeouts.

Test plan:
0. Apply patch
1. cp debian/templates/apache-shared-intranet-plack.conf
/etc/koha/apache-shared-intranet-plack.conf
2. cp debian/templates/apache-shared.conf
/etc/koha/apache-shared.conf
3. service apache2 restart

4. Go to create a SQL report from SQL
/cgi-bin/koha/reports/guided_reports.pl?phase=Create%20report%20from%20SQL
5. Save a report with the following SQL:
SELECT barcode FROM items where barcode <> '';
6. Run the report
7. Download as CSV
8. Edit the CSV and remove the "barcode" heading

9. Open F12 dev tools in browser and open the "Network" tab
10. Go to http://localhost:8081/cgi-bin/koha/mainpage.pl
11. Note that mainpage.pl has a "Content-Encoding: gzip" header
12. Go to http://localhost:8081/cgi-bin/koha/tools/inventory.pl
13. Note that inventory.pl does not have a "Content-Encoding: gzip"
header, while the .js and .css files do have this header.

14. Click "Choose file" and choose your barcode CSV file
15. Uncheck "Compare barcodes list to results"
16. Check "Export to CSV file"
17. Click "Submit"

18. Note that the download initially only increases in size by 1B at a time
19. Open inventory.csv with a text editor that can show symbols like Notepad++
20. Note approximately 7 or 8 NULL characters at start of file
21. Note that CSV loads without problems in Excel or LibreOffice (ie the NULL
characters are ignored)

22. Return to your browser and uncheck "Export to CSV file"
23. Click "Submit"
24. On the "Response" tab, note approximately 7 or 8 NULL characters at start
of response

--

25. For bonus points, set up a reverse proxy in front of inventory.pl
with a proxy timeout of 10 seconds and run an inventory job. Note that
the proxy times out without the patch, and does *not* time out with the patch.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 20 Emily Lamancusa (emlam) 2023-08-04 20:20:12 UTC
Thanks for working on this, David! We've definitely encountered the timeout error, and a keep-alive of sorts is a nice first step for improving the inventory tool!

Testing notes:
I performed steps 1-2 from inside KTD with elevated permissions (also, the paths in steps 1-2 are from the starting point of /kohadevbox/koha).
Comment 21 Marcel de Rooy 2023-08-25 09:18:42 UTC
Hmm. I wouldnt like us to go this road for all scripts that take too much time..
Comment 22 David Cook 2023-08-27 23:39:21 UTC
(In reply to Marcel de Rooy from comment #21)
> Hmm. I wouldnt like us to go this road for all scripts that take too much
> time..

I agree. It's not a good long-term solution, but inventory.pl has had this problem so long and no one has had time to do a rewrite. I think because inventory is run so rarely it fails to be a priority.
Comment 23 Marcel de Rooy 2023-08-28 06:31:43 UTC
(In reply to David Cook from comment #22)
> (In reply to Marcel de Rooy from comment #21)
> > Hmm. I wouldnt like us to go this road for all scripts that take too much
> > time..
> 
> I agree. It's not a good long-term solution, but inventory.pl has had this
> problem so long and no one has had time to do a rewrite. I think because
> inventory is run so rarely it fails to be a priority.

Well. If you use it with some care, you wont get into timeouts.
Comment 24 David Cook 2023-08-28 23:54:58 UTC
(In reply to Marcel de Rooy from comment #23)
> (In reply to David Cook from comment #22)
> > I agree. It's not a good long-term solution, but inventory.pl has had this
> > problem so long and no one has had time to do a rewrite. I think because
> > inventory is run so rarely it fails to be a priority.
> 
> Well. If you use it with some care, you wont get into timeouts.

Corporate proxies become a problem because they will timeout after about 60 seconds. Even with care, libraries will end up with timeouts. 

Performance for inventory was getting worse as well, although with bug 33989 I got inventory working about twice as fast, which was good.
Comment 25 Marcel de Rooy 2023-09-01 08:05:03 UTC
(In reply to David Cook from comment #22)
> (In reply to Marcel de Rooy from comment #21)
> > Hmm. I wouldnt like us to go this road for all scripts that take too much
> > time..
> 
> I agree. It's not a good long-term solution, but inventory.pl has had this
> problem so long and no one has had time to do a rewrite. I think because
> inventory is run so rarely it fails to be a priority.

I would like to get some more feedback from QA.
Comment 26 Jonathan Druart 2023-09-01 08:33:45 UTC
That's kind of a joke. We spent months/years to remove what was in this list, and now we are going to add some more??
Comment 27 Jonathan Druart 2023-09-01 08:38:02 UTC
We have the same problem for report. We need to use the job queue for those ones.
Comment 28 David Cook 2023-09-03 23:39:13 UTC
(In reply to Jonathan Druart from comment #26)
> That's kind of a joke. We spent months/years to remove what was in this
> list, and now we are going to add some more??

I know it's painful. It's the only option I can see aside from using the job queue. 

(In reply to Jonathan Druart from comment #27)
> We have the same problem for report. We need to use the job queue for those
> ones.

Agreed. But I don't think anyone is willing to sponsor the work and inventory.pl isn't a well understood script, so I think it's a bit intimidating to people.

I think we will need to put some thought into supporting its CSV downloads as well. We'll need to prepare the file but only allow people with inventory permissions to download it, and we'll need to add a cleanup job for older inventory results/exports. 

It seems like too much work for too little gain? But it does need to be done eventually somehow...