Summary: | Memory (L1) cache is not flushed before API requests | ||
---|---|---|---|
Product: | Koha | Reporter: | Julian Maurice <julian.maurice> |
Component: | REST API | Assignee: | Julian Maurice <julian.maurice> |
Status: | RESOLVED FIXED | QA Contact: | Jonathan Druart <jonathan.druart> |
Severity: | normal | ||
Priority: | P5 - low | CC: | alexbuckley, dcook, didier.gautheron, fridolin.somers, jesse, jonathan.druart, katrin.fischer, laurent.ducos, lisette, lucas, m.de.rooy, martin.renvoize, nick, paul.derscheid, philippe.blouin, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38630 | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
25.05.00,24.11.02,24.05.07,23.11.12,22.11.24
|
|
Circulation function: | |||
Attachments: |
Bug 38454: [DO NOT PUSH] Add API route to showcase the bug
Bug 38454: Flush memory cache before every API request Bug 38454: Flush memory cache before every API request Bug 38454: Tidy Bug 38454: Flush memory cache before every API request |
Description
Julian Maurice
2024-11-15 10:06:25 UTC
Created attachment 174554 [details] [review] Bug 38454: [DO NOT PUSH] Add API route to showcase the bug This route fetches a value from the memory cache, increment it, store it in memory cache and return the incremented value. If the memory cache was flushed before every request, the route should always return "1". Instead it returns "1" at the first request, "2" at the second request, and so on Test plan: 1. Start the REST API app: `api/v1/app.pl daemon -l http://*:3000` 2. Use curl to request the app: `curl http://localhost:3000/memlite-counter` Created attachment 174557 [details] [review] Bug 38454: Flush memory cache before every API request Test plan: 1. Apply first patch only, follow the test plan in comment 1, witness the bug 2. Apply 2nd patch, follow the test plan in comment 1 again, see that the bug is gone. This is an interesting one! I was going to say it shouldn't be a problem since plack.psgi redefines CGI::new and flushes the cache there, but that should only affect the Staff Interface and OPAC which still use CGI objects. But yeah the API shouldn't be using CGI objects. -- I think the test plan looks a bit alien to Koha devs/users, so I think that could be improved, but yeah... interesting. I don't know how this hasn't come up before. -- I have some other morning tasks to do, but I'll keep this one in my inbox and take another look later... Got sidetracked by other security things... Will have to come back to this one. Test plan: 0. Apply the first patch "Bug 38454: [DO NOT PUSH] Add API route to showcase the bug" 1. Change '/memlite-counter' in Koha/REST/V1.pm to '/api/v1/memlite-counter' 2. sudo koha-plack --reload kohadev 3. Go to http://localhost:8081/api/v1/memlite-counter a few times 4. Note that the counter keeps increasing -- I don't love that this test patch doesn't follow our usual methods, so it's not a perfect test. But... it looks good enough I think, and I can't see anything specific to the REST API that would cause any flushing... We already have a "before_dispatch" hook. Why not add the cache clearing there rather than int he "around_action" hook? (In reply to David Cook from comment #7) > We already have a "before_dispatch" hook. Why not add the cache clearing > there rather than int he "around_action" hook? before_dispatch is called even when no action is run, for instance, before a static file request (it will never happen in the context of the REST api but there is probably other cases). around_action is called right before an action so it seems like the best place. Also it's what is already used in Koha::App::{Intranet,Opac} (In reply to David Cook from comment #6) > I don't love that this test patch doesn't follow our usual methods, so it's > not a perfect test. What do you mean ? (In reply to Julian Maurice from comment #9) > (In reply to David Cook from comment #6) > > I don't love that this test patch doesn't follow our usual methods, so it's > > not a perfect test. > What do you mean ? That is, you didn't add the route via the OpenAPI plugin, so it's not a perfect test of a route Koha would have. But it's not like we're doing anything special with the "openapi_routes_added" hook for our routes. So it shouldn't matter. Created attachment 174760 [details] [review] Bug 38454: Flush memory cache before every API request Signed-off-by: David Cook <dcook@prosentient.com.au> Created attachment 174761 [details] [review] Bug 38454: Tidy Signed-off-by: David Cook <dcook@prosentient.com.au> In any case, it works (more or less) as described, and I think it makes sense as a change overall. (In reply to David Cook from comment #10) > That is, you didn't add the route via the OpenAPI plugin, so it's not a > perfect test of a route Koha would have. Thanks for explaining. I agree it would have been better, but I was in a rush and I always struggle with the openapi schema... I didn't want to postpone this again This patch highlights a bug in how language is handled in the REST API: bug 38630. It would be nice to have them pushed at the same time, but not mandatory. I'm planning to use caching in a Plack Middleware, and I realise that the L1 cache won't be automatically flushed there either... And any script that uses CGI instead of Starman also won't have L1 cache flushed. At the moment that would include: /cgi-bin/koha/tools/export.pl /cgi-bin/koha/tools/inventory.pl /cgi-bin/koha/tools/upload-cover-image.pl /cgi-bin/koha/svc/cataloguing/metasearch (In reply to David Cook from comment #16) > I'm planning to use caching in a Plack Middleware, and I realise that the L1 > cache won't be automatically flushed there either... Actually, since Koha::Cache doesn't implement Memcached's increment method, I'll just bypass the Koha::Cache methods anyway, and work directly with the Cache::Memcached::Fast::Safe object. At least for my proof of concept... (In reply to David Cook from comment #16) > And any script that uses CGI instead of Starman also won't have L1 cache > flushed. At the moment that would include: But scripts in CGI context always start with an empty L1 cache, right ? (In reply to Julian Maurice from comment #18) > (In reply to David Cook from comment #16) > > And any script that uses CGI instead of Starman also won't have L1 cache > > flushed. At the moment that would include: > But scripts in CGI context always start with an empty L1 cache, right ? Yes. (In reply to Julian Maurice from comment #18) > (In reply to David Cook from comment #16) > > And any script that uses CGI instead of Starman also won't have L1 cache > > flushed. At the moment that would include: > But scripts in CGI context always start with an empty L1 cache, right ? Ah right. Phew. Anyway, I don't want to distract from these patches in any case. It would be good to get this fixed. Created attachment 176673 [details] [review] Bug 38454: Flush memory cache before every API request Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Would be nice to have a test but I don't have the time right now to write it. Also dropped the tidy patch. Hi all, We have applied the second patch ('Bug 38454: Flush memory cache before every API request') on this bug report to 24.05.x-security branch Note: We ran through the test plan in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c6 before and after applying the 'Bug 38454: Flush memory cache before every API request' patch. After applying this patch the http://localhost:8081/api/v1/memlite-counter page did not keep incrementing when you refreshed it. We also tried running through the test plan in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c3 however the curl command output does not look expected or match what we see for this page http://localhost:8081/api/v1/memlite-counter. Please let us know if we should remove the 'Bug 38454: Flush memory cache before every API request' patch from the 24.05.x-security branch (In reply to Alex Buckley from comment #24) > We also tried running through the test plan in > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=38454#c3 however > the curl command output does not look expected or match what we see for this > page http://localhost:8081/api/v1/memlite-counter. Are you accessing Koha through a reverse proxy ? If so, then it is expected that the curl command in comment 3 does not work > Please let us know if we should remove the 'Bug 38454: Flush memory cache > before every API request' patch from the 24.05.x-security branch If the test plan in comment 6 works, you can keep the patch. This looks great to me. (In reply to Julian Maurice from comment #25) > If the test plan in comment 6 works, you can keep the patch. Great, thanks for your help! Applied to 23.11.x-security Applied to 22.11.x-security branch for next release. Applied to 24.11.x-security Pushed for 25.05! Well done everyone, thank you! |