Fix memory leak with skin scripts

[djay]

No description provided.

[coveralls]

Pull Request Test Coverage Report for Build 13370359216

Details

• 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 81.597%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/z3c/jbot/patches.py	0	2	0.0%

Totals
Change from base Build 13354810667:	0.0%
Covered Lines:	395
Relevant Lines:	470

---

💛 - Coveralls

[davisagli]

Would it also work to use aq_base(obj)? That might be slightly safer in case something doesn't have a meaningful repr

[djay]

@davisagli I'd rather get an understanding of what its trying to do from someone who designed it. It seems to be a cache to prevent the more expensive looking up of if there is a matching override for this particular script.

1. if its a cache then shouldn't it have a limit where some drop out so it doesn't grow depending on every path within hte site?
2. shouldn't a cache use a weakref so if memory is tight it can be dropped?
3. would it make any difference if all that was stored is the path? this code is getting called by traversal and I can't think of what else in that object is going to change the outcome? Maybe someone deleted the script or changed it? But surely there is a better way of matching than just keeping the object in memory? And a version of it for every possible path in the side since displayContents is called using aquisition for every unique path in the site

[djay]

@davisagli I think that it's also possible it would work the same if the key was the path of the script rather than the url path.

[jensens]

At least using the (possibly persistent) object as a cache key is a no go. This is the entry to hell. It must be pure luck we had not any problems so far. . Good catch!

I am just not sure if the repr is good enough. In theory it should be OK, but I am not aware what are the edge cases we could run into.

[djay]

@jensens @davisagli I've changed the key to the filesystem filename. I can't really think of a reason this is not ok but you guys might be able to.
There is a problem that overriding skin scripts seems to have no tests and I'm not sure its a commonly used feature.

[djay]

@davisagli is using plone as a test dependency ok? otherwise I'm struggling to come up with a test for skins

[jensens]

@davisagli is using plone as a test dependency ok? otherwise I'm struggling to come up with a test for skins

Skins are actually a concept of CMFCore, so it should be enough for testing purposes.
For some inspiration you may want to look at it's tests there https://github.com/zopefoundation/Products.CMFCore/blob/master/src/Products/CMFCore/tests/test_SkinsTool.py

[djay]

@jensens I already looked there. it doesn't test running a script. Only the skin name. So far I actually didn't find a test of the functionality of skins

[djay]

I tested it on the 5.0 site thats been giving me problems. collective.memleak seems to show its fixed this problem but annoyingly the topref counts in the zmi debug control panel still seem to show the objects are there which I can't explain.

[jensens]

I understand, testing here is complex and there are no tests on this level available anyway. I would say it is save to merge this PR, since this is a very simple change.
Any other opinions?

@djay please add an entry to the CHANGES.rst, this is in my opinion the only missing bit preventing a merge.

[djay]

@jensens ok. Shame this also didn't fix my major memory leak like I thought it would :(

[icemac]

@mauritsvanrees @ale-rt Could one of you also review this PR. I can create a release after merging it, if you think this PR is okay. Otherwise I'll create the release without this PR.

[ale-rt]

LGTM, just a suggestion :)