Trashbin memory

[jvillafanez]

Description

Reduce memory usage in the TrashExpire background job

Related Issue

https://github.com/owncloud/enterprise/issues/2925

Motivation and Context

There have been problems related to excesive memory usage, probably caused by an abnormally big trashbin.
Note that problems with excesive group caching has also been detected, but the fix isn't implemented here yet. In addition, the Trashbin::getLocations calls should also be changed.

How Has This Been Tested?

Manually tested with 3 users and a bunch of files inside a couple of folders. The code has been slightly changed to get the peak memory usage in several places.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:

Open tasks:

• Backport (if applicable set "backport-request" label and remove when the backport was done)

[codecov]

Codecov Report

Merging #35094 into master will decrease coverage by 0.07%.
The diff coverage is 2.24%.

@@             Coverage Diff              @@
##             master   #35094      +/-   ##
============================================
- Coverage     65.37%   65.29%   -0.08%     
- Complexity    18626    18650      +24     
============================================
  Files          1215     1215              
  Lines         70532    70618      +86     
  Branches       1295     1295              
============================================
  Hits          46112    46112              
- Misses        24046    24132      +86     
  Partials        374      374

Flag	Coverage Δ	Complexity Δ
#javascript	52.85% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	66.73% <2.24%> (-0.1%)	18650 <22> (+24)

Impacted Files	Coverage Δ	Complexity Δ
...s/files_trashbin/lib/BackgroundJob/ExpireTrash.php	30% <0%> (-1.04%)	13 <0> (+2)
lib/private/Files/Cache/Cache.php	89.36% <0%> (-8.75%)	112 <7> (+7)
apps/files_trashbin/lib/Helper.php	38.14% <0%> (-30.38%)	29 <13> (+13)
apps/files_trashbin/lib/Trashbin.php	76.02% <0%> (-1.68%)	148 <2> (+2)
lib/private/Preview.php	80.2% <100%> (+0.08%)	178 <0> (ø)	⬇️
lib/private/Server.php	86.56% <0%> (-0.12%)	253% <0%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b884aac...767051b. Read the comment docs.

[codecov]

Codecov Report

Merging #35094 into master will decrease coverage by 16.78%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #35094       +/-   ##
=============================================
- Coverage     65.68%    48.9%   -16.79%     
=============================================
  Files          1221      109     -1112     
  Lines         70793    10566    -60227     
  Branches       1288     1288               
=============================================
- Hits          46503     5167    -41336     
+ Misses        23913     5022    -18891     
  Partials        377      377

Flag	Coverage Δ	Complexity Δ
#javascript	53.69% <ø> (ø)	0 <ø> (ø)	⬇️
#phpunit	38.29% <ø> (-28.78%)	0 <ø> (-18735)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/DAV.php	59.45% <0%> (-21.64%)	0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1097 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d59587...b4b2af3. Read the comment docs.

[micbar]

@jvillafanez @pako81 Is this code still needed? I see in the original ticket that you need a new ticket?
What about this one?
If valid, it would need more test coverage.

[pako81]

@micbar it is still under investigation. We are trying to verify if 10.2.0, which should already come with some improvements in terms of memory usage, is already solving the issue. If not, it may be required to apply this code and check the outcome.
@jvillafanez correct me if I am wrong

[jvillafanez]

It's on my radar. I got side-tracked with other more urgent things.
The code is still needed because it fixes a couple of problems regarding high memory usage, but it's under investigation if we need more changes or not.

[jvillafanez]

I don't think I can add more tests here:

• deleteFileIfExpired in "Trashbin.php" relies on the self::delete function, which access to the DB and creates 2 new View objects. In addition, it does a lot of things. Not only the dependencies can't be mocked, but also we'll need access both to the DB and the FS. Unittests are impossible to make here without a heavy refactor.
• getTrashFilesGenerator in "Helper.php". It was copied from the getTrashFiles and adjusted to use a generator. Most of the copied code can't be unittested. There is a new View object that can't be mocked, and the Trashbin::getLocations calls depends on access to the DB, so we can't mock that call.
• run in the "ExpireTrash.php" depends a couple of static code. Those calls aren't isolated and depend on several components, so we can't add unittests there.

The rest of the changes should be already covered by previous tests.

[jvillafanez]

Not sure how to proceed here. There are several problems with the tests due to a confusing architecture around the Files/Cache.

Options:

• Remove the change around Files/Cache/Cache (no more generator). We'll keep the rest of the changes. This will increase the memory usage, but it should fix CI and it also keep the changes at minimum.
• Spread the getFolderContentsGenerator and getFolderContentsByIdGenerator to all the Cache subclasses. I need to investigate if this is possible or not, mainly because of how generators work and the expected behaviour of the cache wrappers.
• Try to fix the CacheWrapper and subclasses by NOT inheriting from the Cache class. This probably will break things and we'll have additional things to fix.
• Try to implement a proper decorator pattern. The problem is that we probably won't be able to move from the current code to there without breaking a lot of things.

[jvillafanez]

I think I'll close this PR and create a new one fixing the memory leaks found.

Although using a generator in the trashbin was a good idea to reduce memory usage, it requires changes in the Cache implementation which either needs to be in the interface or we need to downcast to a private implementation.

For consistency, if we downcast to a private implementation (which is wrong because the expected implementation is in core, not in the trashbin app), we'll also need to handle the rest of the implementation somehow, so we'll still need to rely on the interface. Sound better to rely on the interface in the first place.

If we decide to create a new method in the interface, all the implementations needs to provide something similar. This is starting to get out of scope. The additional problem is that the wrappers also need to implement the same method. This method have to return a generator which wraps the returned generator.
This is either reshaping the architecture or adding new functionality, but not fixing a bug.

[jvillafanez]

Simplified version of the expected solution is in #35612
It doesn't have the changes for the trashbin, but the other 2 problems are also fixed there.

[jvillafanez]

@micbar we should probably close this PR in favour of #35612 . As said above, I don't think it's possible to fix the tests without major changes.

[micbar]

@jvillafanez ok for me , please proceed as proposed :-)

[jvillafanez]

Closing in favour of #35612

[phil-davis]

Closed, so backport not needed