vert-x3 / vertx-config

Vert.x Configuration Service
Apache License 2.0
54 stars 64 forks source link

an HttpClient is probably shared between different Verticles #14

Closed ruslansennov closed 7 years ago

ruslansennov commented 7 years ago

Tests shows some problems with HttpClient (see also vert-x3/vertx-consul-client#32):

Jul 06, 2017 8:49:33 PM io.vertx.core.http.impl.ConnectionManager
WARNING: Reusing a connection with a different context: an HttpClient is probably shared between different Verticles

It seems that ConsulConfigStore's constructor and get() method called in different Vertx contexts. I can confirm this by check Vertx.currentContext() instance in both points. This patch ruslansennov/vertx-consul-client@319b78c1661083c188b2a346fbdab1784af46cd3 fixes the problem but I'm not sure that this is right way. ~Maybe the problem is inside WebClient and/or HttpClient implementations~

vietj commented 7 years ago

the problem happens with keep-alive.

what is the code creating the HttpClient ?

vietj commented 7 years ago

does it happen inside the same verticle or from a main/test method ?

ruslansennov commented 7 years ago

does it happen inside the same verticle or from a main/test method ?

It happens from a test methods but there is issue in vertx-consul-client with the same problem. I made some experiments with ConsulConfigStoreTest:

  @Test
  public void getSimpleConfig(TestContext tc) {
    Async async = tc.async();
    createRetriever();
    System.out.println(Vertx.currentContext());  // (1)
    client.putValue("foo/bar", "value", ar -> {
      System.out.println(Vertx.currentContext());  // (2)
      tc.assertTrue(ar.succeeded());
      retriever.getConfig(json2 -> {
        System.out.println(Vertx.currentContext());  // (3)
        assertThat(json2.succeeded()).isTrue();
        JsonObject config2 = json2.result();
        tc.assertTrue(!config2.isEmpty());
        tc.assertEquals(config2.getString("bar"), "value");
        client.deleteValues("foo", h -> async.complete());
      });
    });
  }

If I run the test by executing the entire class, I see the above warning and:

mvn test -Dtest=ConsulConfigStoreTest

(1) null
WARNING: Reusing a connection with a different context: an HttpClient is probably shared between different Verticles
(2) io.vertx.core.impl.EventLoopContext@1ba3412b
(3) io.vertx.core.impl.EventLoopContext@37c2490c
WARNING: Reusing a connection with a different context: an HttpClient is probably shared between different Verticles

But if I run the getSimpleConfig test only, then I see no warnings and:

mvn test -Dtest=ConsulConfigStoreTest#getSimpleConfig

(1) null
(2) io.vertx.core.impl.EventLoopContext@512f413d
(2) io.vertx.core.impl.EventLoopContext@512f413d

@vietj @cescoffier any thoughts on this?

vietj commented 7 years ago

I think the best would be to capture a Context in consul client when it is created and then use runOnContext to use the HttpClient.

cescoffier commented 7 years ago

I'm not sure it should be done in the consul client or in the config store. For vault I'm doing it in the config store.

ruslansennov commented 7 years ago

Thank you guys. Due to the issues not belongs to vertx-config, I decided to make all the changes in the parent project