JAVA Thrift library: ___currentMethod statefull variable in async mode

[Peter Cipov <pc...@kerio.com>]

Hello
I have code like this:TNonblockingTransport transport = new TNonblockingSocket("127.0.0.1", 9160, 10000);TAsyncClientManager clientManager = new TAsyncClientManager();TProtocolFactory protocolFactory = new TBinaryProtocol.Factory();
//generated thrift clientZipkinCollector.AsyncClient c = new ZipkinCollector.AsyncClient( protocolFactory, clientManager, transport);//calling methods in other threadsc.Log....
I am using async client with nio support (libthrift 0.9.1). I was looking inside the code and I was surprised about ___currentMethod in https://github.com/apache/thrift/blob/0.9.1/lib/java/src/org/apache/thrift/async/TAsyncClient.java#L28
My first question is the purpose of this variable. 
If it is a check to serialize reads and writes to NIO socket than you have a bug there. In this case you have unprotected critical section on ___currentMethod. It can be mutated from two threads - one is Selector thread in TAsyncClientManager and the second one is main thread calling generated thrift protocol method (in this case Log)
public void Log(List<LogEntry> messages, org.apache.thrift.async.AsyncMethodCallback resultHandler) throws org.apache.thrift.TException { checkReady(); Log_call method_call = new Log_call(messages, resultHandler, this, ___protocolFactory, ___transport); this.___currentMethod = method_call; //<------mutating  ___manager.call(method_call);}
This can lead to unpredictable results.
Can some maintainer confirm this ? I would fill you a bug but at first I want to be sure that I did not missed something crucial.
Either way you should really reconsider using mutable variables in asynchronous environment. This can lead to serious mess.


Regards
Peter CipovJánošík