Summary: | Inventory tool can take a long time and never complete | ||
---|---|---|---|
Product: | Koha | Reporter: | Liz Rea <wizzyrea> |
Component: | Staff interface | Assignee: | 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
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. 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... (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. (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. (In reply to David Cook from comment #4) > (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. This will be addressed by bug 33989 (In reply to David Cook from comment #4) > 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'll attach a patch here for this. If folk don't like it here, I can move it to another report. Running it as CGI instead of Plack makes it take about 40 seconds instead of 30 seconds, but it might be worth it to avoid timing out proxies... This one is being tricky. I was able to get it working with curl on some tests, but having a harder time with a browser and a real workload... (In reply to David Cook from comment #8) > This one is being tricky. I was able to get it working with curl on some > tests, but having a harder time with a browser and a real workload... The issue was compression. It was causing too much buffering and that was leading to a timeout even with my workaround. So going to look at turning off compression for inventory.pl so that it can continuously output data to the client and thus not time out. It's not ideal overall, but it will be a functional interim solution. Btw "LogLevel trace6" was my friend in troubleshooting this one. It made it clearer what Apache was during the request and response... I'm fine tuning this, and realising it won't work as nicely for the CSV export as it does for the HTML response. However, instead of printing a space, printing a null makes for a CSV that Excel is quite happy to show perfectly. Turning off the compression turns out to be remarkably challenging. using the environmental variable of "no-gzip" seems to work when you do a GET but the POST seemed not to disable the compression which leads to problems unless you blast 70 bytes presumably to fill some sort of buffer to get things moving... Tried turning off compression with a bit more of a heavy hand but then it disabled compression for Javascript when it should've let it go untouched... mod_deflate seems a bit buggy overall... So it seems to me that you can provide "no-gzip" and it won't compress the response but it still buffers the response: [Wed Jun 14 23:32:04.176918 2023] [filter:trace2] [pid 15344] mod_filter.c(185): [client 172.28.0.1:44628] Content-Type condition for 'deflate' matched, referer: http://localhost:8081/cgi-bin/koha/tools/inventory.pl [Wed Jun 14 23:32:44.751593 2023] [deflate:trace1] [pid 15344] mod_deflate.c(624): [client 172.28.0.1:44628] Not compressing (no-gzip), referer: http://localhost:8081/cgi-bin/koha/tools/inventory.pl So I reckon we need to not add the DEFLATE filter at all for inventory.pl. Now I tried this yesterday but for some reason it also turned off the compression for the Javascript, so I'm going to have another crack at that today... 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. -- 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. 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...) (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.) 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. 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> 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). Hmm. I wouldnt like us to go this road for all scripts that take too much time.. (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. (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. (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. (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. 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?? We have the same problem for report. We need to use the job queue for those ones. (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... |