Skip to content

Commit 5878406

Browse files
robinkarjohrstrom
authored andcommitted
Fix large remote file uploads (#3739)
* Handle remote uploads as file transfers Apache expects a response to the request within 60 seconds (ProxyTimeout). Avoid that by responding with a transfer for the upload, which the browser will query the status of instead. * Explicitly close RemoteTransfer tempfile Prevents the Tempfile from staying around until garbage collection on error.
1 parent 5f3a068 commit 5878406

File tree

5 files changed

+37
-8
lines changed

5 files changed

+37
-8
lines changed

apps/dashboard/app/controllers/files_controller.rb

+9-2
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,16 @@ def upload
154154
parse_path(upload_path)
155155
validate_path!
156156

157-
@path.handle_upload(params[:file].tempfile)
157+
# Need to remove the tempfile from list of Rack tempfiles to prevent it
158+
# being cleaned up once request completes since Rclone uses the files.
159+
request.env[Rack::RACK_TEMPFILES].reject! { |f| f.path == params[:file].tempfile.path } unless posix_file?
158160

159-
render json: {}
161+
@transfer = @path.handle_upload(params[:file].tempfile)
162+
if @transfer
163+
render "transfers/show"
164+
else
165+
render json: {}
166+
end
160167
rescue AllowlistPolicy::Forbidden => e
161168
render json: { error_message: e.message }, status: :forbidden
162169
rescue Errno::EACCES => e

apps/dashboard/app/javascript/files/file_ops.js

+1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ const EVENTNAME = {
2727

2828

2929
let fileOps = null;
30+
export {fileOps};
3031

3132
jQuery(function() {
3233
fileOps = new FileOps();

apps/dashboard/app/javascript/files/uppy_ops.js

+14
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import XHRUpload from '@uppy/xhr-upload'
44
import _ from 'lodash';
55
import {CONTENTID, EVENTNAME as DATATABLE_EVENTNAME} from './data_table.js';
66
import { maxFileSize, csrfToken, uppyLocale } from '../config.js';
7+
import { fileOps } from './file_ops.js';
78

89
let uppy = null;
910

@@ -102,6 +103,7 @@ jQuery(function() {
102103

103104
uppy.on('complete', (result) => {
104105
if(result.successful.length > 0){
106+
result.successful.forEach(handleUploadSuccess);
105107
reloadTable();
106108
}
107109
});
@@ -179,3 +181,15 @@ function reloadTable() {
179181
$(CONTENTID).trigger(DATATABLE_EVENTNAME.reloadTable,{});
180182
}
181183

184+
// Uploads may return the status of a transfer for remote uploads.
185+
function handleUploadSuccess(result) {
186+
// These extra checks might not be needed.
187+
const body = result?.response?.body;
188+
if (!body || !(typeof body === "object" && !Array.isArray(body) && body !== null)) {
189+
return;
190+
}
191+
if (Object.keys(body).length > 0 && !body.completed) {
192+
fileOps.reportTransfer(body);
193+
}
194+
}
195+

apps/dashboard/app/models/remote_file.rb

+9-2
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,15 @@ def write(content)
7777
end
7878

7979
def handle_upload(tempfile)
80-
# FIXME: upload to the remote asynchronously
81-
RcloneUtil.moveto(remote, path, tempfile.path)
80+
# Start a transfer that moves the Rack tempfile to the remote.
81+
transfer = RemoteTransfer.build(
82+
action: "mv",
83+
files: { tempfile.path => path.to_s },
84+
src_remote: RcloneUtil::LOCAL_FS_NAME,
85+
dest_remote: remote,
86+
tempfile: tempfile
87+
)
88+
transfer.tap(&:perform_later)
8289
end
8390

8491
def mime_type

apps/dashboard/app/models/remote_transfer.rb

+4-4
Original file line numberDiff line numberDiff line change
@@ -43,24 +43,23 @@ class RemoteTransfer < Transfer
4343
end
4444
end
4545

46-
attr_accessor :src_remote, :dest_remote, :filesizes, :transferred
46+
attr_accessor :src_remote, :dest_remote, :tempfile, :filesizes, :transferred
4747

4848
class << self
4949
def transfers
5050
# all transfers stored in the Transfer class
5151
Transfer.transfers
5252
end
5353

54-
def build(action:, files:, src_remote:, dest_remote:)
54+
def build(action:, files:, src_remote:, dest_remote:, tempfile: nil)
5555
if files.is_a?(Array)
5656
# rm action will want to provide an array of files
5757
# so if it is an Array we convert it to a hash:
5858
#
5959
# convert [a1, a2, a3] to {a1 => nil, a2 => nil, a3 => nil}
6060
files = Hash[files.map { |f| [f, nil] }].with_indifferent_access
6161
end
62-
63-
self.new(action: action, files: files, src_remote: src_remote, dest_remote: dest_remote)
62+
self.new(action: action, files: files, src_remote: src_remote, dest_remote: dest_remote, tempfile: tempfile)
6463
end
6564
end
6665

@@ -133,6 +132,7 @@ def perform
133132
errors.add :base, e.message
134133
ensure
135134
self.status = OodCore::Job::Status.new(state: :completed)
135+
tempfile&.close(true)
136136
end
137137

138138
def from

0 commit comments

Comments
 (0)