From 3e763ac81e895d2b4af18af2bcd7a31e25da4cf6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 15 Jun 2016 16:30:24 +0200 Subject: [PATCH 1/2] Add timeouts to make the UI usable again when a remote share is unreachable --- apps/federatedfilesharing/lib/DiscoveryManager.php | 5 ++++- apps/federatedfilesharing/lib/Notifications.php | 4 +++- apps/files_sharing/ajax/external.php | 5 ++++- apps/files_sharing/lib/External/Storage.php | 11 +++++++++-- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/apps/federatedfilesharing/lib/DiscoveryManager.php b/apps/federatedfilesharing/lib/DiscoveryManager.php index 2a4bb4b7f7..bf8dfeb31d 100644 --- a/apps/federatedfilesharing/lib/DiscoveryManager.php +++ b/apps/federatedfilesharing/lib/DiscoveryManager.php @@ -84,7 +84,10 @@ class DiscoveryManager { // Read the data from the response body try { - $response = $this->client->get($remote . '/ocs-provider/'); + $response = $this->client->get($remote . '/ocs-provider/', [ + 'timeout' => 3, + 'connect_timeout' => 3, + ]); if($response->getStatusCode() === 200) { $decodedService = json_decode($response->getBody(), true); if(is_array($decodedService)) { diff --git a/apps/federatedfilesharing/lib/Notifications.php b/apps/federatedfilesharing/lib/Notifications.php index 18212b82c3..575521d564 100644 --- a/apps/federatedfilesharing/lib/Notifications.php +++ b/apps/federatedfilesharing/lib/Notifications.php @@ -287,7 +287,9 @@ class Notifications { $endpoint = $this->discoveryManager->getShareEndpoint($protocol . $remoteDomain); try { $response = $client->post($protocol . $remoteDomain . $endpoint . $urlSuffix . '?format=' . self::RESPONSE_FORMAT, [ - 'body' => $fields + 'body' => $fields, + 'timeout' => 3, + 'connect_timeout' => 3, ]); $result['result'] = $response->getBody(); $result['success'] = true; diff --git a/apps/files_sharing/ajax/external.php b/apps/files_sharing/ajax/external.php index 5cf86087f9..6a0a4dfc06 100644 --- a/apps/files_sharing/ajax/external.php +++ b/apps/files_sharing/ajax/external.php @@ -77,7 +77,10 @@ $externalManager = new \OCA\Files_Sharing\External\Manager( // check for ssl cert if (substr($remote, 0, 5) === 'https') { try { - \OC::$server->getHTTPClientService()->newClient()->get($remote)->getBody(); + \OC::$server->getHTTPClientService()->newClient()->get($remote, [ + 'timeout' => 3, + 'connect_timeout' => 3, + ])->getBody(); } catch (\Exception $e) { \OCP\JSON::error(array('data' => array('message' => $l->t('Invalid or untrusted SSL certificate')))); exit; diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index ca99393a1e..7302de5f93 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -254,7 +254,10 @@ class Storage extends DAV implements ISharedStorage { $client = $this->httpClient->newClient(); try { - $result = $client->get($url)->getBody(); + $result = $client->get($url, [ + 'timeout' => 3, + 'connect_timeout' => 3, + ])->getBody(); $data = json_decode($result); $returnValue = (is_object($data) && !empty($data->version)); } catch (ConnectException $e) { @@ -301,7 +304,11 @@ class Storage extends DAV implements ISharedStorage { // TODO: DI $client = \OC::$server->getHTTPClientService()->newClient(); try { - $response = $client->post($url, ['body' => ['password' => $password]]); + $response = $client->post($url, [ + 'body' => ['password' => $password], + 'timeout' => 3, + 'connect_timeout' => 3, + ]); } catch (\GuzzleHttp\Exception\RequestException $e) { if ($e->getCode() === 401 || $e->getCode() === 403) { throw new ForbiddenException(); From ec968a48e44159c1bb323d6ea3be3388277d7928 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Tue, 21 Jun 2016 17:37:44 +0200 Subject: [PATCH 2/2] Increase fed share timeout from 3 to 10 + unit tests --- .../lib/DiscoveryManager.php | 4 +-- .../lib/Notifications.php | 4 +-- .../tests/DiscoveryManagerTest.php | 25 +++++++++++++++---- apps/files_sharing/ajax/external.php | 4 +-- apps/files_sharing/lib/External/Storage.php | 8 +++--- 5 files changed, 30 insertions(+), 15 deletions(-) diff --git a/apps/federatedfilesharing/lib/DiscoveryManager.php b/apps/federatedfilesharing/lib/DiscoveryManager.php index bf8dfeb31d..25af0a40fd 100644 --- a/apps/federatedfilesharing/lib/DiscoveryManager.php +++ b/apps/federatedfilesharing/lib/DiscoveryManager.php @@ -85,8 +85,8 @@ class DiscoveryManager { // Read the data from the response body try { $response = $this->client->get($remote . '/ocs-provider/', [ - 'timeout' => 3, - 'connect_timeout' => 3, + 'timeout' => 10, + 'connect_timeout' => 10, ]); if($response->getStatusCode() === 200) { $decodedService = json_decode($response->getBody(), true); diff --git a/apps/federatedfilesharing/lib/Notifications.php b/apps/federatedfilesharing/lib/Notifications.php index 575521d564..fefa959ba3 100644 --- a/apps/federatedfilesharing/lib/Notifications.php +++ b/apps/federatedfilesharing/lib/Notifications.php @@ -288,8 +288,8 @@ class Notifications { try { $response = $client->post($protocol . $remoteDomain . $endpoint . $urlSuffix . '?format=' . self::RESPONSE_FORMAT, [ 'body' => $fields, - 'timeout' => 3, - 'connect_timeout' => 3, + 'timeout' => 10, + 'connect_timeout' => 10, ]); $result['result'] = $response->getBody(); $result['success'] = true; diff --git a/apps/federatedfilesharing/tests/DiscoveryManagerTest.php b/apps/federatedfilesharing/tests/DiscoveryManagerTest.php index 73f79b2c16..a9c324f024 100644 --- a/apps/federatedfilesharing/tests/DiscoveryManagerTest.php +++ b/apps/federatedfilesharing/tests/DiscoveryManagerTest.php @@ -77,7 +77,10 @@ class DiscoveryManagerTest extends \Test\TestCase { $this->client ->expects($this->once()) ->method('get') - ->with('https://myhost.com/ocs-provider/', []) + ->with('https://myhost.com/ocs-provider/', [ + 'timeout' => 10, + 'connect_timeout' => 10, + ]) ->willReturn($response); $this->cache ->expects($this->at(0)) @@ -111,7 +114,10 @@ class DiscoveryManagerTest extends \Test\TestCase { $this->client ->expects($this->once()) ->method('get') - ->with('https://myhost.com/ocs-provider/', []) + ->with('https://myhost.com/ocs-provider/', [ + 'timeout' => 10, + 'connect_timeout' => 10, + ]) ->willReturn($response); $expectedResult = '/public.php/MyCustomEndpoint/'; @@ -131,7 +137,10 @@ class DiscoveryManagerTest extends \Test\TestCase { $this->client ->expects($this->once()) ->method('get') - ->with('https://myhost.com/ocs-provider/', []) + ->with('https://myhost.com/ocs-provider/', [ + 'timeout' => 10, + 'connect_timeout' => 10, + ]) ->willReturn($response); $expectedResult = '/public.php/webdav'; @@ -151,7 +160,10 @@ class DiscoveryManagerTest extends \Test\TestCase { $this->client ->expects($this->once()) ->method('get') - ->with('https://myhost.com/ocs-provider/', []) + ->with('https://myhost.com/ocs-provider/', [ + 'timeout' => 10, + 'connect_timeout' => 10, + ]) ->willReturn($response); $expectedResult = '/ocs/v2.php/cloud/MyCustomShareEndpoint'; @@ -171,7 +183,10 @@ class DiscoveryManagerTest extends \Test\TestCase { $this->client ->expects($this->once()) ->method('get') - ->with('https://myhost.com/ocs-provider/', []) + ->with('https://myhost.com/ocs-provider/', [ + 'timeout' => 10, + 'connect_timeout' => 10, + ]) ->willReturn($response); $this->cache ->expects($this->at(0)) diff --git a/apps/files_sharing/ajax/external.php b/apps/files_sharing/ajax/external.php index 6a0a4dfc06..4a7a6096c9 100644 --- a/apps/files_sharing/ajax/external.php +++ b/apps/files_sharing/ajax/external.php @@ -78,8 +78,8 @@ $externalManager = new \OCA\Files_Sharing\External\Manager( if (substr($remote, 0, 5) === 'https') { try { \OC::$server->getHTTPClientService()->newClient()->get($remote, [ - 'timeout' => 3, - 'connect_timeout' => 3, + 'timeout' => 10, + 'connect_timeout' => 10, ])->getBody(); } catch (\Exception $e) { \OCP\JSON::error(array('data' => array('message' => $l->t('Invalid or untrusted SSL certificate')))); diff --git a/apps/files_sharing/lib/External/Storage.php b/apps/files_sharing/lib/External/Storage.php index 7302de5f93..29b9c7b563 100644 --- a/apps/files_sharing/lib/External/Storage.php +++ b/apps/files_sharing/lib/External/Storage.php @@ -255,8 +255,8 @@ class Storage extends DAV implements ISharedStorage { $client = $this->httpClient->newClient(); try { $result = $client->get($url, [ - 'timeout' => 3, - 'connect_timeout' => 3, + 'timeout' => 10, + 'connect_timeout' => 10, ])->getBody(); $data = json_decode($result); $returnValue = (is_object($data) && !empty($data->version)); @@ -306,8 +306,8 @@ class Storage extends DAV implements ISharedStorage { try { $response = $client->post($url, [ 'body' => ['password' => $password], - 'timeout' => 3, - 'connect_timeout' => 3, + 'timeout' => 10, + 'connect_timeout' => 10, ]); } catch (\GuzzleHttp\Exception\RequestException $e) { if ($e->getCode() === 401 || $e->getCode() === 403) {