From: Tim Starling <tstarling@wikimedia.org>
Date: Fri, 15 Jun 2018 11:06:40 +0000 (+1000)
Subject: RESTBagOStuff: improve timeouts and logging
X-Git-Tag: 1.34.0-rc.0~4886^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=c21daeebe01298889b4a95ee212368ec784acf6c;p=lhc%2Fweb%2Fwiklou.git

RESTBagOStuff: improve timeouts and logging

* Provide a sensible default for the HTTP timeout, and make it configurable.
* Also make HTTP proxy and caBundlePath configurable.
* Pass through the logger object to the HTTP client, as in EtcdConfig.
* Do not treat a 404 response code for DELETE or 204 for PUT as errors. The
  doc comment already said that delete() should return true if the item
  was not found.

Change-Id: I6a90a25339c3a433381a5300c8c7c1a644e2a10f
---

diff --git a/includes/libs/objectcache/RESTBagOStuff.php b/includes/libs/objectcache/RESTBagOStuff.php
index d3aa9f5cb1..fc3d575547 100644
--- a/includes/libs/objectcache/RESTBagOStuff.php
+++ b/includes/libs/objectcache/RESTBagOStuff.php
@@ -1,5 +1,7 @@
 <?php
 
+use Psr\Log\LoggerInterface;
+
 /**
  * Interface to key-value storage behind an HTTP server.
  *
@@ -27,6 +29,17 @@
  * @endcode
  */
 class RESTBagOStuff extends BagOStuff {
+	/**
+	 * Default connection timeout in seconds. The kernel retransmits the SYN
+	 * packet after 1 second, so 1.2 seconds allows for 1 retransmit without
+	 * permanent failure.
+	 */
+	const DEFAULT_CONN_TIMEOUT = 1.2;
+
+	/**
+	 * Default request timeout
+	 */
+	const DEFAULT_REQ_TIMEOUT = 3.0;
 
 	/**
 	 * @var MultiHttpClient
@@ -43,18 +56,34 @@ class RESTBagOStuff extends BagOStuff {
 		if ( empty( $params['url'] ) ) {
 			throw new InvalidArgumentException( 'URL parameter is required' );
 		}
-		parent::__construct( $params );
 		if ( empty( $params['client'] ) ) {
-			$this->client = new MultiHttpClient( [] );
+			// Pass through some params to the HTTP client.
+			$clientParams = [
+				'connTimeout' => $params['connTimeout'] ?? self::DEFAULT_CONN_TIMEOUT,
+				'reqTimeout' => $params['reqTimeout'] ?? self::DEFAULT_REQ_TIMEOUT,
+			];
+			foreach ( [ 'caBundlePath', 'proxy' ] as $key ) {
+				if ( isset( $params[$key] ) ) {
+					$clientParams[$key] = $params[$key];
+				}
+			}
+			$this->client = new MultiHttpClient( $clientParams );
 		} else {
 			$this->client = $params['client'];
 		}
+		// The parent constructor calls setLogger() which sets the logger in $this->client
+		parent::__construct( $params );
 		// Make sure URL ends with /
 		$this->url = rtrim( $params['url'], '/' ) . '/';
 		// Default config, R+W > N; no locks on reads though; writes go straight to state-machine
 		$this->attrMap[self::ATTR_SYNCWRITES] = self::QOS_SYNCWRITES_QC;
 	}
 
+	public function setLogger( LoggerInterface $logger ) {
+		parent::setLogger( $logger );
+		$this->client->setLogger( $logger );
+	}
+
 	/**
 	 * @param string $key
 	 * @param int $flags Bitfield of BagOStuff::READ_* constants [optional]
@@ -111,7 +140,7 @@ class RESTBagOStuff extends BagOStuff {
 			'body' => serialize( $value )
 		];
 		list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req );
-		if ( $rcode === 200 || $rcode === 201 ) {
+		if ( $rcode === 200 || $rcode === 201 || $rcode === 204 ) {
 			return true;
 		}
 		return $this->handleError( "Failed to store $key", $rcode, $rerr );
@@ -129,7 +158,7 @@ class RESTBagOStuff extends BagOStuff {
 			'url' => $this->url . rawurlencode( $key ),
 		];
 		list( $rcode, $rdesc, $rhdrs, $rbody, $rerr ) = $this->client->run( $req );
-		if ( $rcode === 200 || $rcode === 204 || $rcode === 205 ) {
+		if ( in_array( $rcode, [ 200, 204, 205, 404, 410 ] ) ) {
 			return true;
 		}
 		return $this->handleError( "Failed to delete $key", $rcode, $rerr );