Error handling and logging improvements #9

jframe · 2019-03-14T23:30:05Z

Improve some error handling and logging in EthFirewall

Ensure can't set the http listen host and ports to the same values as the downstream host and port
Exit with error code if the http server fails to start
Add a request timeout so that we don't wait infinitely for a request to fail
Let Vertx handle any errors with the default error handler instead of just hanging (we will want add our own custom response http status codes at some point)
Improve logging for pass through proxy handler

…ream host and port pair

ethfirewall/src/main/java/tech/pegasys/ethfirewall/Runner.java

CjHare · 2019-03-15T01:00:59Z

...all/src/integration-test/java/tech/pegasys/ethfirewall/jsonrpcproxy/IntegrationTestBase.java

@@ -78,7 +78,7 @@ public static void setupEthFirewall() throws IOException, CipherException {
    httpServerOptions.setPort(serverSocket.getLocalPort());
    httpServerOptions.setHost("localhost");

-    runner = new Runner(transactionSigner, httpClientOptions, httpServerOptions);
+    runner = new Runner(transactionSigner, httpClientOptions, httpServerOptions, 5000);


Suggestion: use the more descriptive TimeUnit JDK class i.e. TimeUnit.SECONDS.toMillis(5)

CjHare · 2019-03-15T01:04:06Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/EthFirewall.java

@@ -49,6 +49,12 @@ public void run() {
      return;
    }

+    if (config.getHttpListenHost().equals(config.getHttpListenHost())


You're using string comparison here for integers, whitespace risk i.e. "1" != "1 "

Changed config to now take InetAddress so that trailing spaces are no longer an issue. Also changed to use loopback as the default same what Pantheon is doing. Seems like a better default for security.

CjHare · 2019-03-15T01:04:55Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/EthFirewallCommandLineConfig.java

+  @Option(
+      names = {"--downstream-http-request-timeout"},
+      description =
+          "Timeout in ms to wait for downstream request to timeout (default: ${DEFAULT-VALUE})",


What is the unit? minutes, seconds, milliseconds, nanoseconds?

Using milliseconds. Was using the ms abbreviation, but changed that to use milliseconds to make it clearer.

CjHare · 2019-03-15T01:05:32Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/EthFirewallCommandLineConfig.java

+      description =
+          "Timeout in ms to wait for downstream request to timeout (default: ${DEFAULT-VALUE})",
+      arity = "1")
+  private Integer downstreamHttpRequestTimeout = 5_000;


Is the bounding appropriate for the time unit i.e.should this type be a 'Long'?

done. should be a long as that is what vertx accepts as the type.

…lliseconds

…llow more accurate comparison of host addresses

rain-on · 2019-03-15T06:09:09Z

ethfirewall/src/test/java/tech/pegasys/ethfirewall/EthFirewallCommandLineConfigTest.java

    assertThat(config.getDownstreamHttpPort()).isEqualTo(5000);
    assertThat(config.getDownstreamHttpRequestTimeout()).isEqualTo(10000);
-    assertThat(config.getHttpListenHost()).isEqualTo("localhost");
+    assertThat(config.getHttpListenHost()).isEqualTo(InetAddress.getLoopbackAddress());


is it worth having different values for listen-host and downstream to prove we haven't copied the same cmdline arg in to 2 different params?

good idea. done.

rain-on · 2019-03-15T06:11:45Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/jsonrpcproxy/JsonRpcHttpService.java

@@ -34,12 +34,12 @@

  private final RequestMapper requestHandlerMapper;
  private final HttpServerOptions serverOptions;
-  private final int httpRequestTimeout;
+  private final Long httpRequestTimeout;


should we use the unboxed versions of this when possible? (and others)

It is better. I was following what we were doing for the other types. Thought maybe pico had problem but it's fine. So now we are using long

rain-on · 2019-03-15T06:39:10Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/RunnerBuilder.java

@@ -41,6 +42,10 @@ public void setServerOptions(final HttpServerOptions serverOptions) {
    this.serverOptions = serverOptions;
  }

+  public void setHttpRequestTimeout(final Long requestTimeout) {


Wanted to leave that there, otherwise I can't tell if it has been set. Though 0 for a timeout value doesn't seem particularly useful anyway.

Changed to long. Validating that timeout > 0 as vertx doesn't allow 0 as a timeout value.

CjHare

Nice use of INetAddress

CjHare · 2019-03-15T07:30:31Z

...all/src/integration-test/java/tech/pegasys/ethfirewall/jsonrpcproxy/IntegrationTestBase.java

@@ -85,7 +85,7 @@ public static void setupEthFirewall() throws IOException, CipherException {
    httpServerOptions.setPort(serverSocket.getLocalPort());
    httpServerOptions.setHost("localhost");

-    runner = new Runner(transactionSigner, httpClientOptions, httpServerOptions);
+    runner = new Runner(transactionSigner, httpClientOptions, httpServerOptions, 5000L);


5000L -> TimeUnit.SECONDS.toMills(5)

done. using Duration version

CjHare · 2019-03-15T07:34:12Z

ethfirewall/src/main/java/tech/pegasys/ethfirewall/EthFirewallConfig.java


  Integer getDownstreamHttpPort();

-  String getHttpListenHost();
+  long getDownstreamHttpRequestTimeout();


long doesn't inform the reader of the units.
How about using Duration instead?
https://docs.oracle.com/javase/8/docs/api/java/time/Duration.html

ASP-147: Sync with upstream

jframe added 9 commits March 14, 2019 14:46

validate that the http host and port pair are different to the downst…

b05d1aa

…ream host and port pair

exit if server can't be deployed

9ae7d8a

improve proxy logging

21f1b91

improve wording of vertx deployment failure

d9b7b36

set request timeout

e7c3418

make req timeout private

eed070b

add request timeout config options

5ef9db5

spotless

216b6f8

add explanatory comment for log handler context.next() call

97dab30

jframe requested review from rain-on and CjHare March 14, 2019 23:49

rain-on reviewed Mar 15, 2019

View reviewed changes

ethfirewall/src/main/java/tech/pegasys/ethfirewall/Runner.java Show resolved Hide resolved

CjHare reviewed Mar 15, 2019

View reviewed changes

jframe added 6 commits March 15, 2019 12:20

use long instead of int for request timeout type

fa69f60

update cmd line unit for request timeout to be explicit it's using mi…

ef680c7

…lliseconds

change host type to InetAddress to provider stronger validation and a…

f8e09ad

…llow more accurate comparison of host addresses

fix intg test

c25579e

Merge branch 'master' into error_handling_logging

8d01a17

spotless

6971656

rain-on reviewed Mar 15, 2019

View reviewed changes

jframe added 2 commits March 15, 2019 16:18

PR comments

1dcbf07

PR comments

3f2888c

rain-on reviewed Mar 15, 2019

View reviewed changes

jframe added 2 commits March 15, 2019 16:41

PR comments - more Longs

2c4eecb

ensure timeout > 0

74fc891

CjHare reviewed Mar 15, 2019

View reviewed changes

jframe added 2 commits March 18, 2019 09:17

ensure timeout > 0

d6d1b92

use duration instead long for request timeout type in config

809f641

rain-on approved these changes Mar 18, 2019

View reviewed changes

jframe merged commit 6c47feb into Consensys:master Mar 18, 2019

jframe deleted the error_handling_logging branch March 18, 2019 00:32

sambacha pushed a commit to freight-trust/ethsigner that referenced this pull request Oct 3, 2020

Merge pull request Consensys#9 from Adhara-Tech/asp-147

5cb1c8d

ASP-147: Sync with upstream

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling and logging improvements #9

Error handling and logging improvements #9

jframe commented Mar 14, 2019

CjHare Mar 15, 2019

jframe Mar 15, 2019

CjHare Mar 15, 2019

jframe Mar 15, 2019

CjHare Mar 15, 2019

jframe Mar 15, 2019

CjHare Mar 15, 2019

jframe Mar 15, 2019

rain-on Mar 15, 2019

jframe Mar 15, 2019

rain-on Mar 15, 2019

jframe Mar 15, 2019

rain-on Mar 15, 2019

jframe Mar 15, 2019

jframe Mar 15, 2019

CjHare left a comment

CjHare Mar 15, 2019

jframe Mar 18, 2019

CjHare Mar 15, 2019

jframe Mar 18, 2019

Error handling and logging improvements #9

Error handling and logging improvements #9

Conversation

jframe commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CjHare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment