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

[WIP] Read files from disk with every request #1256

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

whew
Copy link
Contributor

@whew whew commented Dec 20, 2020

Implements #1059.
I didn't go to great lengths to figure out the best way to do this, but I think it works.

Demo

onionshare

This allows files to be changed and have the changes be reflected on the website without restarting Onionshare
@whew whew requested a review from micahflee as a code owner December 20, 2020 07:04
@micahflee
Copy link
Collaborator

Wow, this is very cool! I will start taking a look soon, and maybe we can get it in the 2.3 release.

"gzip compress the individual file, if it hasn't already been compressed" -> "gzip compress the individual file"
It's gonna be compressed either way
@whew
Copy link
Contributor Author

whew commented Dec 22, 2020

Right now the only change is removing the cache of gzipped filenames in send_base_mode so that with each request it reads, compresses, and sends the file, and then deletes the compressed file.

Files are re-read and gzipped each time their mtime changes. If the file's mtime hasn't changed, the last gzipped version is sent.

BTW I just realized that files already are read from disk with each request if should_use_gzip() is False, so changes are reflected on the website. If you modify a file after Onionshare has served a gzipped version of it, you'll get the modified file with$ curl and the original file with $ curl --compressed. Probably a bug.

There's no change yet to share mode's /download logic, it still compresses everything at the beginning only once. Is it a good idea to do that with every request? If you're sharing a lot of files it might take a long time / be CPU intensive if it has to create a ZIP file with every request. Maybe there should be some kind of check to see if files have been modified like os.path.getmtime or a hash?

uses the new gzip caching function from the last commit
some changes in the last commit were reverting the code back to v2.3.dev1
@whew
Copy link
Contributor Author

whew commented Mar 2, 2021

Share mode's /download works now when you're sharing exactly 1 file.
Something I thought, since the size of the gzip file we're sending may change, self.gzip_filesize might be wrong which'll affect the GUI progress bar.

when gzipping the 1 file initially
@whew
Copy link
Contributor Author

whew commented Mar 2, 2021

When you share more than 1 file or any directories, it zips them all at startup and sends that whenever /download is requested.

The way I see it, if any shared files are modified while the share is up, to be able to reflect the changes when someone requests /download, our options are either:
(1) delete the existing zip file and create a new one from scratch, or
(2) modify the existing zip file.

A potential problem is that this is happening when someone requests /download, and that request could time out if what we do takes too long. In the case where you're sharing lots of files or big files (which apparently is a real use case), solution (1) would be impossible (I tested this, zipping two 1GiB files took 100 seconds).

That leaves (2) but I'm not that familiar with zip files in Python. According to these StackOverflow answers from 2014 you can't modify files in a zip archive directly. And you can't delete files from a zip archive either; that feature request has been in Python's issue tracker since 2009. There is a PR from April 2020 that implements deleting from zip files. something like this maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants