Usage of "synchronized" in vert.x

[Krzysztof Kowalczyk]

Hi,

I was going through vert.x code in regards to https://github.com/eclipse/vert.x/issues/1355 but I found something strange that is not really wrong, but i doubt it is required.

In https://github.com/eclipse/vert.x/blob/master/src/main/java/io/vertx/core/http/impl/HttpClientImpl.java

we have

  public synchronized void close() {

    synchronized (this) {

      checkClosed();

      closed = true;

    }

    if (creatingContext != null) {

      creatingContext.removeCloseHook(closeHook);

    }

    pool.close();

    for (ClientConnection conn : connectionMap.values()) {

      conn.close();

    }

    metrics.close();

  }

Is this intended? 

The synchronized method is practically the same as synchronize(this){...}. So in this particular case we have synchronize block inside of synchronize block and both are locking on the same monitor (this). This is completely legal, as locking is reentrant, but it does not make sense to me. We already have a lock so we will not lock it again. Thus only thing it will add imho are 2 bytecode instructions (explicit monitorenter and monitorexit, "synchronize" on method does not insert those) that would not affect logic but makes it confusing and sligtly less likely to be optimised. Am I missing something?

Regards,

Krzysztof

[Julien Viet]

good catch, it is a problem to fix.

the synchronized(this) is there to move a deadlock bug when closing the pool and the idea was to remove the method synchronized.

I created an issue for it.

--
You received this message because you are subscribed to the Google Groups "vert.x" group.
To unsubscribe from this group and stop receiving emails from it, send an email to vertx+un...@googlegroups.com.
Visit this group at https://groups.google.com/group/vertx.
To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/6802da41-4439-4453-bccd-75081ae6c831%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Nat]

Probably, you can address https://github.com/eclipse/vert.x/issues/1307 while you are at it as well.

[Julien Viet]

there is a bunch of client issues to chase :-)

To view this discussion on the web, visit https://groups.google.com/d/msgid/vertx/1b87079b-6c14-4ef9-866d-8e4014aa5dd4%40googlegroups.com.