Don't load already uploaded files into memory

[G1org1owo]

The current methods for post creation/update work using the full content as a byte array. When working with large files, such as videos, this can be a problem, as the data ends up being duplicated in a few occasions.

The most absurd one is having to create a temporary file for use with ffmpeg, when the original file could have been used all along without even loading it into the python process' memory.

This is a draft that addresses this issue by adding to those methods a new optional parameter, content_file, which is used if content is None or empty. The optimizations are as follows:

• Methods that need to access the actual contents, such as mime.get_mime_type, are implemented in a way that severely limits the total data being read (20 bytes).

• To efficiently implement util.get_md5 and util.get_sha1, I have introduced util.get_checksums_from_file, which loads 2M at a time to update both the md5 and sha1 hashes.

• If content_file is set and content is None or empty, __content_file is set rather than __content, and shutil.copyfile is used to copy the original file into the correct data location. Another improvement here could have been using os.rename, but I was not sure I could operate under the assumption temp files obtained via API upload are always meant to be used only once. I'll gladly take suggestions on this one.

The parameter ordering for posts.create_post is quite weird, but I decided against breaking compatibility with the older signature rather than updating it everywhere as I'm not really sure if there are others who have made external scripts using it like me.

I'm open to any comments or suggestions, especially please tell me if you have a better name for _execute_impl as I'm not really a fan of this naming convention but the only other name I could think of in that moment was _execute2.

[G1org1owo]

Marking this as ready for review as it works as intended for post creation; probably worth doing the same for post updates

[po5]

Thanks, this is an important issue.

I don't like hardcoding contentToken and making so many functions aware of it, temp file tokens are meant to be transparently handled in get_file(). If we want to add the same no-memory-load support to thumbnailToken (which is not necessarily a small file, any size image and even a video can be submitted as a thumbnail, and it will be resized by the thumbnailing system) it would require duplicating much of your code.

You identified an issue with _sync_post_content(), it does silly stuff (for both post creation and update):

1. (if we gave a content or thumbnail token) Reads from disk.
2. Writes out to a file.
3. Reads from that file again. (in generate_post_thumbnail())
4. (Whatever other temp file business from images.py)

Your code addresses steps 1 and 2 only in the case of "post update with a main content temp token".
The file copy is applicable for post creation AND update to both content and thumbnails with temp tokens, and would be beneficial for CoW filesystems.
This is the only part in the code where adding references to file tokens makes sense to me.

---

The writing of temporary files in images.py can be avoided in all cases by using stdin with ffmpeg.
Kind of ridiculous we don't do this already.

---

We still read out the full contents of large uploads into memory when uploading a file directly (e.g. when creating a temp file token).
The root culprit is this:

szurubooru/server/szurubooru/rest/app.py

Lines 43 to 51 in 0a0e5fc

	if "multipart" in env.get("CONTENT_TYPE", ""):
	form = cgi.FieldStorage(fp=env["wsgi.input"], environ=env)
	if not form.list:
	raise errors.HttpBadRequest(
	"ValidationError", "No files attached."
	)
	body = form.getvalue("metadata")
	for key in form:
	files[key] = form.getvalue(key)

Instead, we can use form[key].file to get a streamed file handle.
I think I also identified a potential bug with this code, where duplicate keys will result in weird behavior and likely an exception deeper in szurubooru:

If the submitted form data contains more than one field with the same name, the object retrieved by form[key] is not a FieldStorage or MiniFieldStorage instance but a list of such instances. Similarly, in this situation, form.getvalue(key) would return a list of strings.
- https://docs.python.org/3.9/library/cgi.html

Tangent: The cgi module was removed in Python 3.13, and while legacy-cgi exists, we might want to switch to multipart. It supports Python as old as 3.8 so it doesn't clash with our minimum supported version.

Some functions would have to be updated to work with file handles.
Images will still require a full in-memory copy in several places e.g. image_hash.generate_signature(), but it's not as much of an issue since it excludes videos. It might be worth downsizing images before feeding them into such functions.

---

• Use stdin for ffmpeg/ffprobe instead of util.create_temp_file().
• Copy temp file uploads to their final location instead of writing them from scratch. Put all content/thumbnail temp file token stuff in _sync_post_content().
• Reading large files into memory can be avoided by streaming them from the start. Also saves us from opening new file handles to content_file multiple times like in your code. Just do it once in get_file().

I previously wrote an in-depth review and ran into unfortunate memory corruption that crashed my PC and it got lost... Sorry if I forgot to include some part of my reasoning here.

[po5]

Another improvement here could have been using os.rename, but I was not sure I could operate under the assumption temp files obtained via API upload are always meant to be used only once. I'll gladly take suggestions on this one.

I think we should keep the current behavior just like you did. Stale files will automatically get removed. I guess it kind of sucks for non-CoW filesystems though.

[G1org1owo]

I don't like hardcoding contentToken and making so many functions aware of it, temp file tokens are meant to be transparently handled in get_file()

Agree, in that moment I couldn't find a clean way to do this while maintaining backwards compatibility. After reading your comment I think we might implement a context.get_file_stream() and throw away all the content_file idiocy I came up with lol

Some functions would have to be updated to work with file handles.

The main problem I have with this is potentially breaking external scripts (mainly thinking about certain extensions I made to szuru-admin, I'm fairly certain I'm not alone in this). I have no idea what the project's stance on external scripts/backwards compatibility but I was inclined to believe there existed a "public API" when it came to server functions simply because of the existance of _foo() methods. If this is not something we should be concerned about, then I think it's fine to update in this way.

Images will still require a full in-memory copy in several places e.g. image_hash.generate_signature(), but it's not as much of an issue since it excludes videos. It might be worth downsizing images before feeding them into such functions.

I don't believe this would be much of a problem, as this is only run once on upload and even on large public boards I have rarely seen many postings per minute; I guess the flow most effected by this would be mass-uploading. If downsizing images doesn't severely impair signature quality/duplicates individuation I'm definitely on board with this, but I'd much rather make it a server-side config option tbh.

Tangent: The cgi module was removed in Python 3.13, and while legacy-cgi exists, we might want to switch to multipart. It supports Python as old as 3.8 so it doesn't clash with our minimum supported version.

If this is the only place the cgi module is used I'll gladly remove it to oblivion, otherwise I think this definitely needs to be addressed but deserves its own PR.

Use stdin for ffmpeg/ffprobe instead of util.create_temp_file().
Copy temp file uploads to their final location instead of writing them from scratch. Put all content/thumbnail temp file token stuff in _sync_post_content().
Reading large files into memory can be avoided by streaming them from the start. Also saves us from opening new file handles to content_file multiple times like in your code. Just do it once in get_file().

I'll put down a quick top-down todo list with this as its base, if you agree with it I'll go ahead and put it in the original post to keep track of it

• make context.get_file() return a file stream or create an alternate method context.get_file_stream()
• remove all instances of content_file and either add a content_stream in its place for compatibility or make content a stream type
• Copy temp file uploads to their final location instead of writing them from scratch. Put all content/thumbnail temp file token stuff in _sync_post_content().
  • (already done but will need rework with the rest of the changes)
• Update images._execute() and images._execute_impl() to use stdin instead of creating a temp file
  • this potentially removes the need for _execute_impl(), thank goodness

I'd love to jump on this ASAP but I have just nuked my server by moving the main drive to a different mobo and I can't boot it rn, unless I get it running in the next hour and a half I won't be able to start working on this until tomorrow afternoon (my time).

[G1org1owo]

Here I could just return the result of urlopen but I would lose all the error handling that's done by buffering the result. The stupid way is to just wrap the result in a BytesIO and call it a day; I feel the best way to do this is to create a custom object that extends BufferedIOBase and implements read() on the open file descriptor while still performing all the existing error/permission handling. Do you have a better idea before I shoot myself in the foot with this?

szurubooru/server/szurubooru/func/net.py

Lines 40 to 50 in 0a0e5fc

	try:
	with urllib.request.urlopen(request) as handle:
	while chunk := handle.read(_dl_chunk_size):
	length_tally += len(chunk)
	if length_tally > config.config["max_dl_filesize"]:
	raise DownloadTooLargeError(
	"Download target exceeds maximum. (%d)"
	% (config.config["max_dl_filesize"]),
	extra_fields={"URL": url},
	)
	content_buffer += chunk

[G1org1owo]

Reading large files into memory can be avoided by streaming them from the start. Also saves us from opening new file handles to content_file multiple times like in your code. Just do it once in get_file().

I now see a major problem with this approach: how do we read the streams twice? Currently, content is read fully more than once, meaning that we'd have to seek the stream to its start before being able to do further operations. Now, this is completely doable when it comes to temp uploads, as they are actually files on disk, but what about the handles for direct uploads and URL downloads? I'm not so sure those streams support seeking, especially the one returned by urllib.request.urlopen() (since the docs are not very clear on the actual interface)