Repository: Wikidata/Wikidata-Toolkit (java)

Issue:
Calls to the Wikibase API should throw an exception if no network is available
There does not seem to be any timeout on HTTP requests issued by WDTK. So, if we attempt to make an HTTP request via WDTK while disconnected from the network, the call will stall indefinitely.
It should rather throw an exception after a user-configurable timeout.

Pull request:
Introduce methods to set timeouts for HTTP calls.
Closes #456.

Patch:
diff --git a/wdtk-wikibaseapi/src/main/java/org/wikidata/wdtk/wikibaseapi/ApiConnection.java b/wdtk-wikibaseapi/src/main/java/org/wikidata/wdtk/wikibaseapi/ApiConnection.java
index 1be00562..cdb92336 100644
--- a/wdtk-wikibaseapi/src/main/java/org/wikidata/wdtk/wikibaseapi/ApiConnection.java
+++ b/wdtk-wikibaseapi/src/main/java/org/wikidata/wdtk/wikibaseapi/ApiConnection.java
@@ -193,6 +193,18 @@ public class ApiConnection {
 	 * Map of requested tokens.
 	 */
 	final Map<String, String> tokens;
+	
+	/**
+	 * Maximum time to wait for when establishing a connection, in milliseconds.
+	 * For negative values, no timeout is set.
+	 */
+	int connectTimeout = -1;
+	
+	/**
+	 * Maximum time to wait for a server response once the connection was established.
+	 * For negative values, no timeout is set.
+	 */
+	int readTimeout = -1;
 
 	/**
 	 * Mapper object used for deserializing JSON data.
@@ -772,8 +784,56 @@ public class ApiConnection {
 		connection.setUseCaches(false);
 		connection.setRequestProperty("Content-Type",
 				"application/x-www-form-urlencoded");
+		if(connectTimeout >= 0) {
+			connection.setConnectTimeout(connectTimeout);
+		}
+		if(readTimeout >= 0) {
+			connection.setReadTimeout(readTimeout);
+		}
 		connection.setRequestProperty(ApiConnection.PARAM_COOKIE,
 				getCookieString());
 	}
+	
+	/**
+	 * Maximum time to wait for when establishing a connection, in milliseconds.
+	 * For negative values, no timeout is set, which is the default behaviour (for
+	 * backwards compatibility).
+	 * 
+	 * @see HttpURLConnection.getConnectionTimeout
+	 */
+	public int getConnectTimeout() {
+		return connectTimeout;
+	}
+	
+	/**
+	 * Sets the maximum time to wait for when establishing a connection, in milliseconds.
+	 * For negative values, no timeout is set.
+	 * 
+	 * @see HttpURLConnection.setConnectionTimeout
+	 */
+	public void setConnectTimeout(int timeout) {
+		connectTimeout = timeout;
+	}
+	
+	/**
+	 * Maximum time to wait for a server response once the connection was established.
+	 * For negative values, no timeout is set, which is the default behaviour (for backwards
+	 * compatibility).
+	 * 
+	 * @see HttpURLConnection.getReadTimeout
+	 */
+	public int getReadTimeout() {
+		return readTimeout;
+	}
+	
+	/**
+	 * Sets the maximum time to wait for a server response once the connection was established.
+	 * For negative values, no timeout is set.
+	 * 
+	 * @see HttpURLConnection.setReadTimeout
+	 */
+	public void setReadTimeout(int timeout) {
+		readTimeout = timeout;
+	}
 
 }


Test patch:
diff --git a/wdtk-wikibaseapi/src/test/java/org/wikidata/wdtk/wikibaseapi/BasicApiConnectionTest.java b/wdtk-wikibaseapi/src/test/java/org/wikidata/wdtk/wikibaseapi/BasicApiConnectionTest.java
index c9c395e5..1aea0577 100644
--- a/wdtk-wikibaseapi/src/test/java/org/wikidata/wdtk/wikibaseapi/BasicApiConnectionTest.java
+++ b/wdtk-wikibaseapi/src/test/java/org/wikidata/wdtk/wikibaseapi/BasicApiConnectionTest.java
@@ -20,6 +20,14 @@ package org.wikidata.wdtk.wikibaseapi;
  * #L%
  */
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Matchers.anyInt;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
 import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.URL;
@@ -54,7 +62,7 @@ public class BasicApiConnectionTest {
 			"\"GeoIP\":\"DE:13:Dresden:51.0500:13.7500:v4\",\" path\":\"/\",\" Domain\":\".wikidata.org\","+
 			"\"testwikidatawikiSession\":\"c18ef92637227283bcda73bcf95cfaf5\",\" secure\":\"\","+
 			"\"WMF-Last-Access\":\"18-Aug-2015\",\"Expires\":\"Sat, 19 Sep 2015 12:00:00 GMT\",\"HttpOnly\":\"\","+
-			"\" Path\":\"/\",\" httponly\":\"\"},\"username\":\"username\"}";
+			"\" Path\":\"/\",\" httponly\":\"\"},\"username\":\"username\",\"connectTimeout\":-1,\"readTimeout\":-1}";
 	
 	Set<String> split(String str, char ch) {
 		Set<String> set = new TreeSet<>();
@@ -337,6 +345,45 @@ public class BasicApiConnectionTest {
 		// after a while, the credentials expire
 		this.con.checkCredentials();
 	}
+	
+	/**
+	 * For backwards compatibility: by defaults, no timeouts
+	 * are set by us, we use HttpURLConnection's defaults.
+	 * @throws IOException
+	 */
+	@Test
+	public void testNoTimeouts() throws IOException {
+		HttpURLConnection urlConn = mock(HttpURLConnection.class);
+		
+		this.con.setupConnection("GET", "foo=bar", urlConn);
+		
+		verify(urlConn, times(0)).setConnectTimeout(anyInt());
+		verify(urlConn, times(0)).setReadTimeout(anyInt());
+	}
+	
+	@Test
+	public void testConnectTimeout() throws IOException {
+		HttpURLConnection urlConn = mock(HttpURLConnection.class);
+		
+		this.con.setConnectTimeout(1000);
+		this.con.setupConnection("GET", "foo=bar", urlConn);
+		
+		assertEquals(con.getConnectTimeout(), 1000);
+		verify(urlConn, times(1)).setConnectTimeout(1000);
+		verify(urlConn, times(0)).setReadTimeout(anyInt());
+	}
+	
+	@Test
+	public void testReadTimeout() throws IOException {
+		HttpURLConnection urlConn = mock(HttpURLConnection.class);
+		
+		this.con.setReadTimeout(2000);
+		this.con.setupConnection("GET", "foo=bar", urlConn);
+		
+		assertEquals(con.getReadTimeout(), 2000);
+		verify(urlConn, times(0)).setConnectTimeout(anyInt());
+		verify(urlConn, times(1)).setReadTimeout(2000);
+	}
 
 	private List<String> testCookieList() {
 		List<String> cookieList = new ArrayList<>();