Skip to content

Set HTTPServer request/response timeouts #643

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

Merged

Conversation

perlun
Copy link
Contributor

@perlun perlun commented Mar 17, 2021

Closes #636

Closes prometheus#636

Signed-off-by: Per Lundberg <per.lundberg@hibox.tv>
@perlun perlun force-pushed the feature/add-request-and-response-timeouts branch from 77f0beb to 3e44c73 Compare March 17, 2021 14:30
System.setProperty("sun.net.httpserver.maxRspTime", "600");
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding a test for this as well, like this:

diff --git simpleclient_httpserver/src/test/java/io/prometheus/client/exporter/TestHTTPServer.java simpleclient_httpserver/src/test/java/io/prometheus/client/exporter/TestHTTPServer.java
index c9f6bee..f02dbd4 100644
--- simpleclient_httpserver/src/test/java/io/prometheus/client/exporter/TestHTTPServer.java
+++ simpleclient_httpserver/src/test/java/io/prometheus/client/exporter/TestHTTPServer.java
@@ -13,6 +13,7 @@ import java.util.zip.GZIPInputStream;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import sun.net.httpserver.ServerImplHelper;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.fail;
@@ -147,4 +148,14 @@ public class TestHTTPServer {
     String response = requestWithCompression("/-/healthy", "");
     assertThat(response).contains("Exporter is Healthy");
   }
+
+  @Test
+  public void testRequestTimeout() {
+    assertThat(ServerImplHelper.getRequestTimeout()).isEqualTo(60000);
+  }
+
+  @Test
+  public void testResponseTimeout() {
+    assertThat(ServerImplHelper.getResponseTimeout()).isEqualTo(600000);
+  }
 }
diff --git simpleclient_httpserver/src/test/java/sun/net/httpserver/ServerImplHelper.java simpleclient_httpserver/src/test/java/sun/net/httpserver/ServerImplHelper.java
new file mode 100644
index 0000000..f841435
--- /dev/null
+++ simpleclient_httpserver/src/test/java/sun/net/httpserver/ServerImplHelper.java
@@ -0,0 +1,17 @@
+package sun.net.httpserver;
+
+/**
+ * Helper class to be able to read package-internal fields from {@link ServerImpl}.
+ *
+ * @author Per Lundberg
+ */
+public class ServerImplHelper {
+
+    public static long getRequestTimeout() {
+        return ServerImpl.MAX_REQ_TIME;
+    }
+
+    public static long getResponseTimeout() {
+        return ServerImpl.MAX_RSP_TIME;
+    }
+}

However, this fails with an exception: java.lang.IllegalAccessError: tried to access class sun.net.httpserver.ServerImpl from class sun.net.httpserver.ServerImplHelper. The sun.net.httpserver classes are really not written with (external, non-JDK) testability in mind... 😒

Copy link
Member

@fstab fstab left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I agree that this is hard to test.

@fstab fstab merged commit 29dfd0a into prometheus:master Mar 19, 2021
@perlun perlun deleted the feature/add-request-and-response-timeouts branch March 19, 2021 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support request and response timeouts in HTTPServer
2 participants