Is it possible that there might be some unclosed HTTP streams in the MacOS implementation?

[Ansis100]

I didn't want to start an issue without concrete evidence.

We have a weird issue on MacOS where if the downloader has been uploading files for a long time (about 2.5k uploaded files), then the whole app will become buggy and unusable. We traced it down to an error in a different library (file_picker) mentioning "ProcessException: Too many open files". So the OS prohibits performing any additional file descriptor actions, hence the bugginess.

I was wondering if this might have been cause by background_downloader opening too many file or HTTP streams and not closing them. I ran lsof -c app_process and saw a new IPv4 file descriptor being created for every task that was processed by the downloader. That would be normal, but the descriptors were not disappearing from the list even after the upload was completed. I'm not 100% sure if this is caused by the package, but if it is, it could cause any app to eventually go into a state of not being able to open any further network connections.

[781flyingdutchman]

Interesting and definitely possible - thanks for stress testing the plugin! I'll look through the code and see if this may indeed be the case - which plugin version are you using?

[Ansis100]

In my testing I noticed that all the old connections are CLOSE_WAIT, which supposedly means that something is wrong in the first place.
I'm not sure on the specifics of UNIX sockets, but I'm guessing somewhere, something is needlessly opening a new connection for every upload even though it should be reusing the old connection?

[Ansis100]

I think I found the issue. In my case it was the http.StreamedRequest opening connections in binaryUpload, finishing them with a CLOSE_WAIT, but not removing them.

background_downloader/lib/src/desktop/upload_isolate.dart

Lines 49 to 56 in 3ec71d6

	request.headers.addAll(task.headers);
	request.contentLength = fileSize;
	request.headers['Content-Type'] = task.mimeType;
	request.headers['Content-Disposition'] =
	'attachment; filename="${Uri.encodeComponent(task.filename)}"';
	// initiate the request and handle completion async
	final requestCompleter = Completer();
	var transferBytesResult = TaskStatus.failed;

After adding a request.persistentConnection = false; after line 53, the CLOSE_WAIT descriptors seem to disappear.

I haven't yet tested if it affects the upload connections in any other way. The multipartUpload also uses StreamedRequest, I haven't tested that either.

[781flyingdutchman]

Thanks for looking into this thoroughly. I'll mark this as a bug - do you want to submit a PR for this? You'd have to make sure all integration tests in the example app pass (especially for the upload ones) for all platforms (Windows, MacOS, Linux in particular, given this affects the desktop platforms only) and would indeed suggest doing this also for the multi-part upload. You shoudl base your PR on the dev branch as there is already some other stuff being tested there. If not, let me know and I will add it when I get to it.

[781flyingdutchman]

I looked into this a bit and it seems that it is preferred to set persistentConnection to true, so the client can reuse a connection to the same server. However, the isolate architecture for desktop downloads and uploads forces the creation of a new httpClient for each task, which is likely why you're seeing connections all over the place. It would be great if you could test if setting it to false leads to slower performance if you upload or download a large number of smaller files to the same server. I don't think so, so suggest we change to false for all uploads and downloads on desktop.

[781flyingdutchman]

In my very basic testing I saw no adverse effect of setting persistentConnection to false, and of closing the HTTP client when the isolate exists. Both are likely better from a memory and performance perspective, so adding this to the next release.

[781flyingdutchman]

Implemented in V8.6.0

[Ansis100]

Thank you!
I'll keep an eye on the performance, but our testers didn't report any noticeable difference.