Skip to content
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

Add config variable for curl timeout #38292

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Add config variable for curl timeout #38292

merged 3 commits into from
Jun 13, 2023

Conversation

dfuchss
Copy link
Contributor

@dfuchss dfuchss commented May 15, 2023

Summary

Add the config variable for davstorage.request_timeout in order to define remote timeouts.
Needed for nextcloud federation with large files.

Details on errors:

Basically, you encounter two types of errors when dealing with large files in federated shares (see below).
In both cases you get a timeout because the put / get command is not finished in 30s (hard coded in the defaults; see lib/private/Http/Client/Client.php). This PR introduces the configuration variable davstorage.request_timeout which replaces this default timeout in the get/put command of the DAV module. The DAV module is used by the federated share to transfer data between two cloud instances.

Download from Federated Share:

[webdav] Error: Sabre\DAV\Exception: cURL error 28: Operation timed out after 30000 milliseconds with 315250376 out of 1478242034 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for https://REMOTE_CLOUD/public.php/webdav/SOME_FILE_Cut_copy.mp4 at <<closure>>

0. /var/www/cloud-html/apps/dav/lib/Connector/Sabre/File.php line 492
   OCA\DAV\Connector\Sabre\File->convertToSabreException()
1. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/CorePlugin.php line 85
   OCA\DAV\Connector\Sabre\File->get()
2. /var/www/cloud-html/3rdparty/sabre/event/lib/WildcardEmitterTrait.php line 89
   Sabre\DAV\CorePlugin->httpGet()
3. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 472
   Sabre\DAV\Server->emit()
4. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 253
   Sabre\DAV\Server->invokeMethod()
5. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 321
   Sabre\DAV\Server->start()
6. /var/www/cloud-html/apps/dav/lib/Server.php line 366
   Sabre\DAV\Server->exec()
7. /var/www/cloud-html/apps/dav/appinfo/v2/remote.php line 35
   OCA\DAV\Server->exec()
8. /var/www/cloud-html/remote.php line 172
   require_once("/var/www/cloud- ... p")

Caused by:

GuzzleHttp\Exception\ConnectException: cURL error 28: Operation timed out after 30000 milliseconds with 315250376 out of 1478242034 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for https://REMOTE_CLOUD/public.php/webdav/SOME_FILE_Cut_copy.mp4 at <<closure>>

 0. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlFactory.php line 158
    GuzzleHttp\Handler\CurlFactory::createRejection("*** sensitive parameters replaced ***")
 1. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlFactory.php line 110
    GuzzleHttp\Handler\CurlFactory::finishError()
 2. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlHandler.php line 47
    GuzzleHttp\Handler\CurlFactory::finish()
 3. /var/www/cloud-html/lib/private/Http/Client/DnsPinMiddleware.php line 150
    GuzzleHttp\Handler\CurlHandler->__invoke()
 4. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/PrepareBodyMiddleware.php line 35
    OC\Http\Client\DnsPinMiddleware->OC\Http\Client\{closure}("*** sensitive parameters replaced ***")
 5. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Middleware.php line 31
    GuzzleHttp\PrepareBodyMiddleware->__invoke()
 6. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/RedirectMiddleware.php line 71
    GuzzleHttp\Middleware::GuzzleHttp\{closure}("*** sensitive parameters replaced ***")
 7. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Middleware.php line 63
    GuzzleHttp\RedirectMiddleware->__invoke()
 8. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/HandlerStack.php line 75
    GuzzleHttp\Middleware::GuzzleHttp\{closure}("*** sensitive parameters replaced ***")
 9. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 331
    GuzzleHttp\HandlerStack->__invoke()
10. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 168
    GuzzleHttp\Client->transfer()
11. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 187
    GuzzleHttp\Client->requestAsync("*** sensitive parameters replaced ***")
12. /var/www/cloud-html/lib/private/Http/Client/Client.php line 226
    GuzzleHttp\Client->request()
13. /var/www/cloud-html/lib/private/Files/Storage/DAV.php line 358
    OC\Http\Client\Client->get()
14. /var/www/cloud-html/lib/private/Files/Storage/Wrapper/Wrapper.php line 298
    OC\Files\Storage\DAV->fopen()
15. /var/www/cloud-html/lib/private/Files/Storage/Wrapper/Availability.php line 314
    OC\Files\Storage\Wrapper\Wrapper->fopen()
16. /var/www/cloud-html/apps/files_antivirus/lib/AvirWrapper.php line 77
    OC\Files\Storage\Wrapper\Availability->fopen()
17. /var/www/cloud-html/lib/private/Files/View.php line 1180
    OCA\Files_Antivirus\AvirWrapper->fopen()
18. /var/www/cloud-html/lib/private/Files/View.php line 1006
    OC\Files\View->basicOperation()
19. /var/www/cloud-html/apps/dav/lib/Connector/Sabre/File.php line 490
    OC\Files\View->fopen()
20. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/CorePlugin.php line 85
    OCA\DAV\Connector\Sabre\File->get()
21. /var/www/cloud-html/3rdparty/sabre/event/lib/WildcardEmitterTrait.php line 89
    Sabre\DAV\CorePlugin->httpGet()
22. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 472
    Sabre\DAV\Server->emit()
23. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 253
    Sabre\DAV\Server->invokeMethod()
24. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 321
    Sabre\DAV\Server->start()
25. /var/www/cloud-html/apps/dav/lib/Server.php line 366
    Sabre\DAV\Server->exec()
26. /var/www/cloud-html/apps/dav/appinfo/v2/remote.php line 35
    OCA\DAV\Server->exec()
27. /var/www/cloud-html/remote.php line 172
    require_once("/var/www/cloud- ... p")

Upload to Federated Share:

[no app in context] Error: GuzzleHttp\Exception\ConnectException: cURL error 28: Operation timed out after 30000 milliseconds with 0 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) for https://REMOTE_CLOUD/public.php/webdav/SOME_FILE_Cut.mp4 at <<closure>>

 0. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlFactory.php line 158
    GuzzleHttp\Handler\CurlFactory::createRejection("*** sensitive parameters replaced ***")
 1. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlFactory.php line 110
    GuzzleHttp\Handler\CurlFactory::finishError()
 2. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Handler/CurlHandler.php line 47
    GuzzleHttp\Handler\CurlFactory::finish()
 3. /var/www/cloud-html/lib/private/Http/Client/DnsPinMiddleware.php line 150
    GuzzleHttp\Handler\CurlHandler->__invoke()
 4. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/PrepareBodyMiddleware.php line 64
    OC\Http\Client\DnsPinMiddleware->OC\Http\Client\{closure}("*** sensitive parameters replaced ***")
 5. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Middleware.php line 31
    GuzzleHttp\PrepareBodyMiddleware->__invoke()
 6. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/RedirectMiddleware.php line 71
    GuzzleHttp\Middleware::GuzzleHttp\{closure}("*** sensitive parameters replaced ***")
 7. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Middleware.php line 63
    GuzzleHttp\RedirectMiddleware->__invoke()
 8. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/HandlerStack.php line 75
    GuzzleHttp\Middleware::GuzzleHttp\{closure}("*** sensitive parameters replaced ***")
 9. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 331
    GuzzleHttp\HandlerStack->__invoke()
10. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 168
    GuzzleHttp\Client->transfer()
11. /var/www/cloud-html/3rdparty/guzzlehttp/guzzle/src/Client.php line 187
    GuzzleHttp\Client->requestAsync("*** sensitive parameters replaced ***")
12. /var/www/cloud-html/lib/private/Http/Client/Client.php line 333
    GuzzleHttp\Client->request()
13. /var/www/cloud-html/lib/private/Files/Storage/DAV.php line 514
    OC\Http\Client\Client->put()
14. /var/www/cloud-html/lib/private/Files/Storage/DAV.php line 422
    OC\Files\Storage\DAV->uploadFile()
15. /var/www/cloud-html/lib/private/Files/Storage/DAV.php line 413
    OC\Files\Storage\DAV->writeBack()
16. <<closure>>
    OC\Files\Storage\DAV->OC\Files\Storage\{closure}("*** sensitive parameters replaced ***")
17. /var/www/cloud-html/3rdparty/icewind/streams/src/CallbackWrapper.php line 119
    call_user_func()
18. <<closure>>
    Icewind\Streams\CallbackWrapper->stream_close("*** sensitive parameters replaced ***")
19. /var/www/cloud-html/lib/private/Files/Storage/Common.php line 883
    fclose("*** sensitive parameters replaced ***")
20. /var/www/cloud-html/lib/private/Files/Storage/Wrapper/Wrapper.php line 644
    OC\Files\Storage\Common->writeStream()
21. /var/www/cloud-html/lib/private/Files/Storage/Wrapper/Wrapper.php line 644
    OC\Files\Storage\Wrapper\Wrapper->writeStream()
22. /var/www/cloud-html/apps/files_antivirus/lib/AvirWrapper.php line 96
    OC\Files\Storage\Wrapper\Wrapper->writeStream()
23. /var/www/cloud-html/apps/dav/lib/Connector/Sabre/File.php line 248
    OCA\Files_Antivirus\AvirWrapper->writeStream()
24. /var/www/cloud-html/apps/dav/lib/Connector/Sabre/Directory.php line 149
    OCA\DAV\Connector\Sabre\File->put()
25. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Tree.php line 307
    OCA\DAV\Connector\Sabre\Directory->createFile("*** sensitive parameters replaced ***")
26. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Tree.php line 133
    Sabre\DAV\Tree->copyNode()
27. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Tree.php line 163
    Sabre\DAV\Tree->copy()
28. /var/www/cloud-html/apps/dav/lib/Upload/ChunkingPlugin.php line 94
    Sabre\DAV\Tree->move()
29. /var/www/cloud-html/apps/dav/lib/Upload/ChunkingPlugin.php line 76
    OCA\DAV\Upload\ChunkingPlugin->performMove()
30. /var/www/cloud-html/3rdparty/sabre/event/lib/WildcardEmitterTrait.php line 89
    OCA\DAV\Upload\ChunkingPlugin->beforeMove()
31. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/CorePlugin.php line 603
    Sabre\DAV\Server->emit()
32. /var/www/cloud-html/3rdparty/sabre/event/lib/WildcardEmitterTrait.php line 89
    Sabre\DAV\CorePlugin->httpMove()
33. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 472
    Sabre\DAV\Server->emit()
34. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 253
    Sabre\DAV\Server->invokeMethod()
35. /var/www/cloud-html/3rdparty/sabre/dav/lib/DAV/Server.php line 321
    Sabre\DAV\Server->start()
36. /var/www/cloud-html/apps/dav/lib/Server.php line 366
    Sabre\DAV\Server->exec()
37. /var/www/cloud-html/apps/dav/appinfo/v2/remote.php line 35
    OCA\DAV\Server->exec()
38. /var/www/cloud-html/remote.php line 172
    require_once("/var/www/cloud- ... p")

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label May 15, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone May 15, 2023
@solracsf solracsf added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 15, 2023
@kesselb
Copy link
Contributor

kesselb commented May 16, 2023

Similar pr: #35461

@dfuchss
Copy link
Contributor Author

dfuchss commented May 16, 2023

Similar pr: #35461

Thanks for the pointer to the PR. Actually, that was one I haven't found :)

@dfuchss
Copy link
Contributor Author

dfuchss commented May 22, 2023

Currently, I use the PR for discussions and as base for testing with my local instances.
The problem we've encountered was between two nextcloud instances while transferring a big file (~10 GB) via nextcloud federation:

cURL error 28: Operation timed out after 30000 milliseconds with 119422620 out of 1478242034 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) Therefore, we assume that it's related to the 30s default timeout.

@szaimen
Copy link
Contributor

szaimen commented May 23, 2023

Currently, I use the PR for discussions and as base for testing with my local instances. The problem we've encountered was between two nextcloud instances while transferring a big file (~10 GB) via nextcloud federation:

cURL error 28: Operation timed out after 30000 milliseconds with 119422620 out of 1478242034 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html) Therefore, we assume that it's related to the 30s default timeout.

I see. However this is not the correct place to fiy this. You will need to find the correct place e.g. in the files_sharing or files_external app and add your code there.

@dfuchss
Copy link
Contributor Author

dfuchss commented May 23, 2023

The good news: I was able to verify that these two lines of code are able to resolve the issues with federation shares and large files. Therefore, you can simply set the value of remote_curl_timeout to a suitable amount of time > 30s. That's some kind of hotfix, but resolves the issue for me and my friends.

The question is now whether this is the right location for the change.
I'd argue that the uploadFile method may be a good location for such a timeout.
Nevertheless, I'm not sure about the pendant (get in fopen).
I'd be happy to get some suggestions and opinions on where to put it finally (I'd also be happy if the location is ok :) )
Since I'm not a PHP developer and I'm dealing with the nextcloud source code for the first time, any suggestion could help me.

@szaimen szaimen requested review from kesselb, come-nc, a team, icewind1991 and nfebe and removed request for a team May 23, 2023 17:25
@szaimen szaimen added 3. to review Waiting for reviews and removed 2. developing Work in progress labels May 23, 2023
@szaimen szaimen marked this pull request as ready for review May 23, 2023 17:26
Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your pull request 👍

Add a configuration option to increase the timeout for dav requests seems reasonable to me.

lib/private/Files/Storage/DAV.php Outdated Show resolved Hide resolved
lib/private/Files/Storage/DAV.php Outdated Show resolved Hide resolved
config/config.sample.php Outdated Show resolved Hide resolved
@dfuchss
Copy link
Contributor Author

dfuchss commented Jun 11, 2023

Thanks for your pull request 👍

Add a configuration option to increase the timeout for dav requests seems reasonable to me.

I hope the changes fit from your point of view.

@kesselb
Copy link
Contributor

kesselb commented Jun 12, 2023

Index: lib/private/Files/Storage/DAV.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php
--- a/lib/private/Files/Storage/DAV.php	(revision 45590e45cca81a1405826dd01733fd31b98fa1f8)
+++ b/lib/private/Files/Storage/DAV.php	(date 1686559195855)
@@ -51,6 +51,7 @@
 use OCP\Files\StorageNotAvailableException;
 use OCP\Http\Client\IClientService;
 use OCP\ICertificateManager;
+use OCP\IConfig;
 use OCP\Util;
 use Psr\Http\Message\ResponseInterface;
 use Sabre\DAV\Client;
@@ -139,7 +140,7 @@
 		$this->logger = \OC::$server->get(LoggerInterface::class);
 		$this->eventLogger = \OC::$server->get(IEventLogger::class);
 		// This timeout value will be used for the download and upload of files
-		$this->timeout = \OC::$server->getConfig()->getSystemValueInt('davstorage.request_timeout', 30);
+		$this->timeout = \OC::$server->get(IConfig::class)->getSystemValueInt('davstorage.request_timeout', 30);
 	}
 
 	protected function init() {

To replace the deprecated getConfig.

@dfuchss
Copy link
Contributor Author

dfuchss commented Jun 12, 2023

Index: lib/private/Files/Storage/DAV.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/private/Files/Storage/DAV.php b/lib/private/Files/Storage/DAV.php
--- a/lib/private/Files/Storage/DAV.php	(revision 45590e45cca81a1405826dd01733fd31b98fa1f8)
+++ b/lib/private/Files/Storage/DAV.php	(date 1686559195855)
@@ -51,6 +51,7 @@
 use OCP\Files\StorageNotAvailableException;
 use OCP\Http\Client\IClientService;
 use OCP\ICertificateManager;
+use OCP\IConfig;
 use OCP\Util;
 use Psr\Http\Message\ResponseInterface;
 use Sabre\DAV\Client;
@@ -139,7 +140,7 @@
 		$this->logger = \OC::$server->get(LoggerInterface::class);
 		$this->eventLogger = \OC::$server->get(IEventLogger::class);
 		// This timeout value will be used for the download and upload of files
-		$this->timeout = \OC::$server->getConfig()->getSystemValueInt('davstorage.request_timeout', 30);
+		$this->timeout = \OC::$server->get(IConfig::class)->getSystemValueInt('davstorage.request_timeout', 30);
 	}
 
 	protected function init() {

To replace the deprecated getConfig.

I'll handle the suggestion asap. Probably, after work :)

@szaimen
Copy link
Contributor

szaimen commented Jun 12, 2023

3rdparty Outdated Show resolved Hide resolved
@dfuchss
Copy link
Contributor Author

dfuchss commented Jun 12, 2023

Add the config variable for curl calls ("remote_curl_timeout"). E.g., needed for nextcloud federation.

Signed-off-by: Dominik Fuchß <develop@fuchss.org>
Signed-off-by: Dominik Fuchß <develop@fuchss.org>
Signed-off-by: Dominik Fuchß <develop@fuchss.org>
@dfuchss
Copy link
Contributor Author

dfuchss commented Jun 13, 2023

@kesselb what did you changed from 57c4d41 to e3f6a13 ?

@kesselb
Copy link
Contributor

kesselb commented Jun 13, 2023

@dfuchss I let GitHub rebase your pull request to pull in the latest changes from master. This should give us a successful CI run.

Copy link
Contributor

@kesselb kesselb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine code wise. Didn't test it.

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but didnt test as well

@szaimen szaimen enabled auto-merge June 13, 2023 09:07
@szaimen szaimen merged commit 4b3c552 into nextcloud:master Jun 13, 2023
@welcome
Copy link

welcome bot commented Jun 13, 2023

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@szaimen
Copy link
Contributor

szaimen commented Jun 13, 2023

Docu in nextcloud/documentation#10618

@dfuchss dfuchss deleted the feature/configurabe_timeout branch June 13, 2023 19:24
@spyfly
Copy link

spyfly commented Jun 14, 2023

Is this going to get backported to stable26 and stable25?

@szaimen
Copy link
Contributor

szaimen commented Jun 14, 2023

@kesselb ? 🤷

@kesselb
Copy link
Contributor

kesselb commented Jun 14, 2023

/backport to stable27

@kesselb
Copy link
Contributor

kesselb commented Jun 14, 2023

It looks like a reasonable and not too risky change, therefore a backport for 27 and 26 should work. Strictly speaking, it's a new feature, which we usually do not backport. I'm unable to make this choice.

@szaimen
Copy link
Contributor

szaimen commented Jun 14, 2023

Lets only backport to 27 then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NC 20] "cURL error 28: Operation timed out" when uploading big files to nonlocal federation share
5 participants