-
Notifications
You must be signed in to change notification settings - Fork 8
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
[upload] browser crashes while trying to upload large file (~600Mb) #89
Comments
@mariorodeghiero can we close this? Seems has been solved in #90? |
I think yes. |
@mariorodeghiero after some tests in NHS, the following error is throwing trying to upload the file ~600Mb: |
@risenW @mariorodeghiero I'm using this file, you can try this also - just download and exract the csv file from archive: LMK if it works for you locally. |
@mbeilin Sorry about my earlier comment, I was using a smaller file instead. Currently working on the issue now. |
@mbeilin This issue has been fixed and merged with the master branch. You can try it from your end. cc: @mariorodeghiero |
great, let's test it. |
@mbeilin we found the cause of the problem. It's because of the buffer method. We'll work on it. |
@risenW can we also get a reasonable local (automated) test with a large file so that we increase likelihood of finding issues here rather than when integrating into upstream code? (NB: the large file might need to be stored outside of git ...) |
@rufuspollock Yes, I'll set this up locally. |
How we can test this in upload large file function? In #96 it's not described |
@kmanaseryan i don't quite get
what does that mean? Why does this mean it won't be handled by the storage service. could you give a bit more detail. |
I am a bit lost here, but here is what is needed in order to support large file uploads to Git LFS based storage as is implemented by
Here is what axios accepts as upload data:
I'm really not much of a JS expect, so I can't say which one works best for us (maybe a different type depending on node vs browser) but I would be happy to jump on a call to figure this out. |
Ok, after reading @kmanaseryan's analysis again I understand that some of these will not be supported in older browsers (IE 11 for example, and from what I can see even modern versions of Safari may be problematic but I'm not 100% sure on this one). In this case, if we need to support these browsers, the only option I see going forward is that uploading large files will only be possible with the |
@shevron thanks for the insights.
This is already implemented both for node and browser. We have
So after the recent updates now
We can implement a new method which will do this behind the scene using streams.
As for Axios, in the browser I've used FormData and File as data for the payload, but it didn't work, i.e the HTTP request for uploading to blob storage didn't failed, but the verification failed. In node haven't tried. |
Closing this, you can follow datopian/ckan-client-js#32 |
Browser crashes while trying to upload a large CSV file (~600Mb) for the resource
Acceptance
Tasks
Analysis
There are two options:
Uploading streams - Problem: stream upload is not supported by old browsers, since we should use Fetch API
[Udpates]: This is not applicable. We should use FormData() and the file stream appened on it which doens't meet the way it suppoused to be done for upload file to the storage. Currently we pass a buffer, not a FormData object having a bufer/file/stream, this won't be handled by the storage service.
Multipart - Problem: We can directly modify queries etc of upload storage request, however this depends on which storage we use, for each provider we have to make custom changes, which will basically endup having our own Uppy plugin.
The text was updated successfully, but these errors were encountered: