Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enh(metadata): Introduce a memory limit for metadata generation #42800

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Jan 15, 2024

Summary

Fix #42793

By introducing a (configurable) memory limit, one should not reach OOM on very big files that the server can't support.

TODO

  • Once approved, document metadata_max_filesize param

Similar to:

/**
* max memory for generating image previews with imagegd (default behavior)
* Reads the image dimensions from the header and assumes 32 bits per pixel.
* If creating the image would allocate more memory, preview generation will
* be disabled and the default mimetype icon is shown. Set to -1 for no limit.
*
* Defaults to ``256`` megabytes
*/
'preview_max_memory' => 256,

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Jan 15, 2024
@solracsf solracsf added this to the Nextcloud 29 milestone Jan 15, 2024
@solracsf solracsf requested a review from artonge January 16, 2024 20:59
Copy link
Member

@joshtrichards joshtrichards left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, but I'm not familiar enough with the metadata stuff to officially review (though code looks reasonable).

Suggestion: Add the new parameter to config.sample.php here too

@solracsf
Copy link
Member Author

Suggestion: Add the new parameter to config.sample.php here too

It's the ToDo point on the PR summary 😉

Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I forgot to submit the review

core/BackgroundJobs/GenerateMetadataJob.php Outdated Show resolved Hide resolved
This was referenced Mar 12, 2024
This was referenced Mar 20, 2024
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@kesselb
Copy link
Contributor

kesselb commented Jun 16, 2024

Thank you, good idea 👍

I think the decision "can this file be processed" should live in the actual event listeners. But the event listeners I checked all read the file, and therefore it's a practical approach.

The naming is a bit misleading because it's not a memory limit but a file size limit.

@solracsf
Copy link
Member Author

solracsf commented Jun 16, 2024

The naming is a bit misleading because it's not a memory limit but a file size limit.

If metadata_max_filesize fits best please let me know.

@kesselb
Copy link
Contributor

kesselb commented Jun 26, 2024

The naming is a bit misleading because it's not a memory limit but a file size limit.

If metadata_max_filesize fits best please let me know.

That's good, thank you 👍

@solracsf solracsf requested a review from kesselb June 27, 2024 06:50
@kesselb
Copy link
Contributor

kesselb commented Jun 27, 2024

Please don't forget to update the config.sample.php

I'm not sure how useful the debug log is, but without you have no idea why the metadata generation was skipped for a given file.

@solracsf solracsf added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jun 28, 2024
@st3iny
Copy link
Member

st3iny commented Jul 31, 2024

/backport to stable29

Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amended to fix the commit message.

@st3iny st3iny enabled auto-merge July 31, 2024 13:00
@kesselb
Copy link
Contributor

kesselb commented Jul 31, 2024

Psalm is failing because the latest rebase brought in #46450 that changed IConfig to IAppConfig.

@solracsf solracsf disabled auto-merge July 31, 2024 16:48
@blizzz blizzz mentioned this pull request Aug 1, 2024
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Co-Authored-By: Louis <louis@chmn.me>
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud>
@st3iny st3iny enabled auto-merge August 5, 2024 06:48
@st3iny st3iny merged commit 8511b89 into master Aug 5, 2024
169 checks passed
@st3iny st3iny deleted the metaGenMemLimit branch August 5, 2024 07:06
@Altahrim Altahrim mentioned this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish feature: previews and thumbnails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cronjob went OOM once a day, I suspect GenerateMetadataJob.
6 participants