facebookarchive / swift

An annotation-based Java library for creating Thrift serializable types and services.
Apache License 2.0
900 stars 297 forks source link

What is the best practice for calling a client method concurrently? #243

Closed xielong closed 6 years ago

xielong commented 9 years ago

The following code establishes hundreds of connections:

final ThriftClientManager clientManager = new ThriftClientManager();
final FramedClientConnector connector = new FramedClientConnector(fromParts("localhost", port));

for (int i = 0; i < numThreads; i++){
    new Thread(){
        public void run(){
            Scribe scribe = clientManager.createClient(connector, Scribe.class).get();
            scribe.log(Collections.<LogEntry>emptyList()));
        }
    }.start();
}
xielong commented 9 years ago

I tried changing transports and protocols in ThriftInvocationHandler to thread local variables, recompiled swift-service:

private ThreadLocal<TChannelBufferInputTransport> inputTransportHolder = new ThreadLocalTChannelBufferInputTransport>(){
    @Override
    protected TChannelBufferInputTransport initialValue() {
        return new TChannelBufferInputTransport();
    }
};
...

then I can make PRC calls like this:

final ThriftClientManager clientManager = new ThriftClientManager();
final FramedClientConnector connector = new FramedClientConnector(fromParts("localhost", port));
Scribe scribe = clientManager.createClient(connector, Scribe.class).get();

for (int i = 0; i < numThreads; i++){
    new Thread(){
        public void run(){
            scribe.log(Collections.<LogEntry>emptyList()));
        }
    }.start();
}
andrewcox commented 9 years ago

If, in the interface declaration, you change the return type for your method to be a ListenableFuture, instead of just Type, the call will happen asynchronously.

From: xielong notifications@github.com<mailto:notifications@github.com> Reply-To: facebook/swift reply@reply.github.com<mailto:reply@reply.github.com> Date: Friday, January 9, 2015 at 2:07 AM To: facebook/swift swift@noreply.github.com<mailto:swift@noreply.github.com> Subject: Re: [swift] What is the best practice for calling a client method concurrently? (#243)

I tried to change transports and protocols in ThriftInvocationHandler to thread local variables, recompiled swift-service:

private ThreadLocal inputTransportHolder = new ThreadLocalTChannelBufferInputTransport>(){ @Override protected TChannelBufferInputTransport initialValue() { return new TChannelBufferInputTransport(); } }; ...

then I can make a PRC call like this:

final ThriftClientManager clientManager = new ThriftClientManager(); final FramedClientConnector connector = new FramedClientConnector(fromParts("localhost", port)); Scribe scribe = clientManager.createClient(connector, Scribe.class).get();

for (int i = 0; i < numThreads; i++){ new Thread(){ public void run(){ scribe.log(Collections.emptyList())); } }.start(); }

— Reply to this email directly or view it on GitHubhttps://github.com/facebook/swift/issues/243#issuecomment-69315431.

xielong commented 9 years ago

Sorry for unclear description. I mean that the scribe created by ClientManager is not thread safe, so I need to create one client per thread:

for (int i = 0; i < 80; i++){
    new Thread(){
        public void run(){
            Scribe scribe = clientManager.createClient(connector, Scribe.class).get();
            ListenableFuture<Long> future = scribe.log(entryList);
            Futures.addCallback(future, new FutureCallback<Long>() {
                @Override
                public void onSuccess(Long result) {
                    System.out.println(result);
                }

                @Override
                public void onFailure(Throwable t) {
                    t.printStackTrace();
                }
            });
        }
    }.start();
}

But clientManager.createClient is a heavy operation, in the code above, it connects to the server 80 times and creates a lot of connections. After using ThreadLocal variables to store transports and protocols in ThriftInvocationHandler, multi-threads can share the same client and the same connection.

electrum commented 9 years ago

You should be able to share the same client between threads as long as you are not calling it concurrently from multiple threads. Thus, for example, sharing a client with a lock should work.

Ideally, Swift would have a connection pool, then you could use a simple try-with-resources block to acquire a client and it would perform well even under high load.

For Scribe, we have an a Scribe library that logs to Scribe asynchronously. This has the advantage of not blocking callers and allowing retry if the Scribe service is down (such as when it is being restarted for an upgrade). It also avoids the problem you are seeing by flushing in batches from a single background thread. I don't think it has anything Facebook specific in it, so we can discuss open sourcing it if you're interested.

xielong commented 9 years ago

Thanks electrum. The Scribe client sounds greate. Back to Swift. Since Swift is based on Netty, maybe we don't need a connection pool. The client created by ClientManager is not thread safe because ThriftInvocationHandler which uses transports and tprotocols is not thread safe. It seems that using ThreadLocal variables to store transports and protocols in ThriftInvocationHandler is a simple workaround.

ghost commented 9 years ago

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

supercharger commented 9 years ago

Can you open source scribe library.