hprose / hprose-java

Hprose is a cross-language RPC. This project is Hprose 2.0 for Java
MIT License
550 stars 187 forks source link

HproseTcpClient may miss the first request after connected #32

Closed xiaoyur347 closed 6 years ago

xiaoyur347 commented 6 years ago

The step to reproduce:

  1. The server is in loopback mode or the server is in the same machine with the client.
  2. call IProtocol protocol = Client.useService("0.0.0.0:port").
  3. call protocol.func() only once to synchronize call to the server. Sometimes this call will fail.

Reason: The client will wait HproseClient.timeout before it fails, and the default time is 5000ms or 5s.

Since it is the first call, size = 0, it will call ConnectorHolder.create() to connect to server. When the server lives in the same machine with the client, maybe onConnected() is called before requests.offer(request); And the connection becomes idle! If we do rpc call again, it has the chance to communicate, but if we only communicate once, it may fail forever.

protected final void create(Request request) {
        if (size.get() < client.getMaxPoolSize()) {
            try {
                ConnectorHolder.create(client.uri, this, client.isKeepAlive(), client.isNoDelay());
            }
            catch (IOException ex) {
                request.result.reject(ex);
//                while ((request = requests.poll()) != null) {
//                    request.result.reject(ex);
//                }
                return;
            }
        }
        requests.offer(request);
    }
public final void onConnected(Connection conn) {
        Request request = requests.poll();
        if (request != null) {
            send(conn, request);
        }
        else { //come here
            synchronized (idleConnections) {
                if (!idleConnections.contains(conn)) {
                    idleConnections.offer(conn);
                }
            }
            recycle(conn);
        }
    }

The problem is idleConnections lock range is too small for onConnected(). And I suggest this:

public final void onConnected(Connection conn) {
    synchronized (idleConnections) {
        Request request = requests.poll();
        if (request != null) {
            send(conn, request);
        }
        else {
                if (!idleConnections.contains(conn)) {
                    idleConnections.offer(conn);
                }
            recycle(conn);
        }
    }
}

Since create() is only called by fetch(), and fetch() has a big lock for idleConnections. And we can share this lock to avoid the problem.

andot commented 6 years ago

Yes, you are right. Thank you! You can create a pull request, I will merge it.