TUS temporary file optimization (mv instead of read on same filesystem)

[JeffersonBledsoe]

• this is an optimisation that bypasses reading and writing the TUS upload if it's on the same file system as the tmp dir used for new blobs. This speeds up the last part of a TUS upload considerably.
• it also bypasses DX validation of the blob as that seems to read this into memory for some reason.
  • instead we should work out how to still validate but not read the data unless we really have to

Plan

• does rename from tmp to blob instead of reading and writing, if on same file system
• avoid reading in whole blob during NamedBlobFile validation (slows TUS uploads) plone.namedfile#155 - get this or another fix merged
• put back in DX file validation but after we prevent it from reading in whole file for default case
• put in tests
• document somewhere that its important that the temp dir and blob dir are in the same filesystem to prevent slow copy of the file

[netlify]

✅ Deploy Preview for plone-restapi canceled.

Name	Link
🔨 Latest commit	dabf6d9
🔍 Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/65601089e392d1000863a250

[mister-roboto]

@JeffersonBledsoe thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[davisagli]

@JeffersonBledsoe NamedBlobFile uses IStorage adapters to process the value that is passed in (https://github.com/plone/plone.namedfile/blob/master/plone/namedfile/storages.py). It might make sense to implement this as an IStorage adapter for TUSUpload.

it also bypasses DX validation of the blob as that seems to read this into memory for some reason. instead we should work out how to still validate but not read the data unless we really have to

Yeah, I don't think we can bypass validation entirely. If the entire file is read during validation that's surprising, have you identified where that is happening?

[djay]

@davisagli I've put in the storage adapter and that works. I've also put in a test to show it works.

But the validation does actually read the whole file into memory (also shown in the test).

The cause is

• part of validating a field is to validate all the attributes of the field against it's schema
• so data property in the namedblobFile instance is being validated against INamedBlobFile and schema.Bytes field default validation.
• but data is a dynamic property which will read the entire file into memory
• and it does this for no reason since all it actually checks is that it has a min length of 0
  • it maybe checks it;s of type bytes but doesn't a file have to be anyway?
  • but it actually doesn't matter what the validation. it will read the data in regardless - get_schema_validation_errors - as long as Bytes field is validatable the data is read even if no validation is performed

Note this is a problem for NamedImageBlob too. and this is likely a problem for any editing of a File object or other reasons files might be validated

Possible solutions?

• we skip default validation in NamedBlobFile field and instead do our own sanity check that the NamedBlobFile value is what it says it is?
• maybe use something other than a bytes field in the INamedBlobFile schema taht doesn't do validation against file contents itself?
  • I removed the bytes schema definition from IFile and the test passed so the file wasn't read into memory.

Not really sure the right way to solve this yet. ideas?

[djay]

I also suspect that some of the other storages like FileUploadStorage might also be be already buffered into a local file at least some of the time. In that case, the file can be moved rather than read again improving performance. I haven't looked into this yet however.

[djay]

@davisagli I had a go at changing the validation to avoid reading in the whole file in plone/plone.namedfile#155

[davisagli]

Suggested change

	raise NotStorable('Could not store data (not of "FileUpload").')
	raise NotStorable('Could not store data (not of type "TUSUpload").')

[davisagli]

@djay Now that I'm looking at things more carefully, I'm wondering if this special storage adapter is really needed.

Previously, an open file was passed into the NamedBlobFile constructor. This should end up using the FileDescriptorStorable which uses blob.consumeFile, which is already using rename_or_copy_blob.

[davisagli]

This should go in a finally block in case there's an error during the test.