CODEWITHSUNDEEP
javaapache-httpclient-4.xsynchronized
L.Tian
L.Tian

Reputation: 63

why IdleConnectionMonitorThread need to synchronize

I see many examples of IdleConnectionMonitorThread using 'synchronized' in implement of 'run'. In my mind, it makes no sense. The implement in

edu.uci.ics crawler4j 4.2 

public class IdleConnectionMonitorThread extends Thread {

  private final PoolingHttpClientConnectionManager connMgr;
  private volatile boolean shutdown;

  public IdleConnectionMonitorThread(PoolingHttpClientConnectionManager connMgr) {
    super("Connection Manager");
    this.connMgr = connMgr;
  }

  @Override
  public void run() {
    try {
      while (!shutdown) {
        synchronized (this) {
          wait(5000);
          // Close expired connections
          connMgr.closeExpiredConnections();
          // Optionally, close connections that have been idle longer than 30 sec
          connMgr.closeIdleConnections(30, TimeUnit.SECONDS);
        }
      }
    } catch (InterruptedException ignored) {
      // terminate
    }
  }

  public void shutdown() {
        shutdown = true;
        synchronized (this) {
            notifyAll();
        }
        log.warn("newPosition: shutdown idleMonitorThread");

    }

}

Since in most cases we only have one IdleConnectionMonitorThread and use it in this way, synchronized (this) has no meaning.

IdleConnectionMonitorThread idleConnectionMonitorThread = new IdleConnectionMonitorThread(poolingHttpClientConnectionManager)

I wonder the benefit of using 'synchronized', can use implement run in this way (delete the synchronized)

      @Override
      public void run() {
        try {
          while (!shutdown) {
              wait(5000);
              // Close expired connections
              connMgr.closeExpiredConnections();
              // Optionally, close connections that have been idle longer than 30 sec
              connMgr.closeIdleConnections(30, TimeUnit.SECONDS);
          }
        } catch (InterruptedException ignored) {
          // terminate
        }
      }

Upvotes: 1

Views: 1457

Answers (1)

Stephen C
Stephen C

Reputation: 719386

Read the javadocs for Object.wait(long).

If you call it when you don't hold the mutex you are waiting on, you will get an exception. The javadoc states:

Throws: IllegalMonitorStateException - if the current thread is not the owner of the object's monitor.

In general, locking the mutex in the "wait/notify" protocol is essential to ensure that the shared state that is being managed is visible to all threads that are participating. In this case the shutdown variable is declared as volatile so that isn't a concern. However, the "wait/notify" protocol requires locking anyway; see above.

I wonder the benefit of using synchronized, can use implement run in this way (delete the synchronized).

Nope. If you deleted the synchronized you would get an exception. But you could replace Object.wait(...) with Thread.sleep(...) ... and then the synchronized would be unnecessary.

As to the "oddness" of this code, who knows why the author ended up with this. But does it really matter? There is an Engineering principle: "If it ain't broken, don't fix it". Nobody has proposed a reason why this code is broken.

Upvotes: 1

Related Questions