Desktop: slow webdav sync on linux

[e-ihrke]

This is a proposal to fix the Issue #1023 by using an http.Agent with a configured keepAlive.

I had to delete the agent from the options within the serializeRequest method because it has a reference to the request which also has a reference to the agent again, which would result in an error while calling stringify.

I could test this fix only on my linux desktop.

[laurent22]

In which way does it solve the issue? Have you been able to test it?

Also since it's a fix for Linux only, it should be scoped to Linux using shim.isLinux()

[e-ihrke]

I've had the issue on my linux desktop and could fix the problem with these changes. So yes, I've tested it.

Since it is usually good to have a connection keep alive I would rather not scope it in a way that only linux users have it. But if you want to have the check there I can add it.

The only thing I'm not sure of here is how high/low the "keepAliveMsecs" option should be.

[tessus]

This code touches all platforms. Not that your solution might not work, but what would happen, if the phone goes to sleep? Or would this prevent the phone from sleeping?
I really think that this has to be guarded better, or more research is needed to know all the consequences of this change.

[laurent22]

I agree with @tessus. This code is used by all the operating systems supported by Joplin and is meant to work with many different WebDAV servers, so it's risky to apply the change to everything.

From experience, even seemingly harmless changes sometimes hit a glitch of the OS network stack or of the WebDAV server. So unless there are proven performance improvements to all platforms, let's restrict the change to just Linux. Later, if someone can take the time to test and benchmark other platforms, we can apply the patch to more platforms.

Also please add a comment on the code that explains what the change does, and linking it to issue #1023.

[e-ihrke]

Sorry for not responding during the last days, I had a short vacation.

Over the weekend I had the opportunity to test the keepAlive on a windows system though and it increased the performance of the sync notably. (Though I tested a fullsync only with my testnotes three times with and without the changes each.)

If you want to make sure that these changes would work for every webdav server that is stated in the readme, it would not make sense to enable it for linux either. I would not think that the problems which could be introduced with the keep alive are on the client (OS) side, but rather on the server side, when it has not implemented this (quiet old) feature.

For the mobile app I do not know if this could be a problem. After the sync is done the connection would be closed after 5 seconds which should be ok then. But it should definitively be tested before just enabling it.

Instead of having a single hack for linux in the webdav synchronization code, I would suggest to introduce an option to the webdav configuration, so that the user can decide to use / try it or not.

Would this be ok for you? If yes should it be done in another PR? (Should the option be labeled with: “Try connection reuse”?)

I will add a comment to my changes (I guess in the constructor) which will also link to the stated issue.
Also I’ve added my findings to the issue itself, where I explain why I’ve fallen back to this workaround.

[laurent22]

If it's a good change then we should enable it for all users instead of adding an option. The option implies that each user would have to test and benchmark it, which is unnecessary. If we do it ourselves then it's done once and for all, and no need for an option.

Actually the app makes many connections when syncing so with your change it could mean a very big speed improvement as we get rid of the connection overhead. I'll try to think of some way to benchmark it to confirm. Once we confirmed it improve performances, we can then test on various platforms, which shouldn't be too hard.

You're right that testing on every WebDAV server doesn't make much sense and we can assume that if it works with one it will work with others, since this is more of a low-level change.

[laurent22]

One thing I've noticed is that this change is compatible only with http URLs (not https) because that's what the http package supports. So there has to be some logic to create either an http or an https agent depending on the WebDAV URL.

Also I wonder if the http package is available at all on React Native. It seems like a simple change but there's still quite a bit of work to do unfortunately, so unless someone can look into it I won't be able to merge.

[laurent22]

For the record it is 30% faster the agent:

Without keep alive

Created remote items: 336. Fetched items: 39/50.
Created remote items: 336. Fetched items: 90/100.
Created remote items: 336. Fetched items: 145/150.
Created remote items: 336. Fetched items: 202/250.
Created remote items: 336. Fetched items: 259/300.
Created remote items: 336. Fetched items: 326/336.
Downloading resources...
Created remote items: 336. Fetched items: 336/336. Completed: 10/10/2019 20:01

real    7m4.744s
user    0m5.453s
sys     0m4.625s

With keep alive

Created remote items: 336. Fetched items: 178/200.
Created remote items: 336. Fetched items: 216/250.
Created remote items: 336. Fetched items: 260/300.
Created remote items: 336. Fetched items: 306/336.
Downloading resources...
Created remote items: 336. Fetched items: 336/336. Completed: 10/10/2019 20:09

real    5m9.336s
user    0m3.969s
sys     0m3.063s

[laurent22]

It's a good change but there's a lot more work to do, so I'm going to close it for now. Feel free to comment here to re-open if you find the time to complete the PR.

[maxadamo]

Can't we get this feature into Linux only (or excluding the mobile platforms only), at least as temporary hack, to be reverted in the future?
I don't see the point of letting the Linux users have a very bed user experience.

[tessus]

@maxadamo as you can see from Laurent's comment, the code is not complete. It would only work for http, which is hopefully not used by anyone. In this state, we cannot merge this PR.
It also does not limit this change to Linux.