[jira] [Commented] (GEODE-7573) Issues with TrasactionIds managed by CacheTransactionManager in C++ native client

["Jacob Barrett (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/GEODE-7573?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17000991#comment-17000991 ] 

Jacob Barrett commented on GEODE-7573:
--------------------------------------

The transactions support in the C++ client is in a very bad place. Any changes you want to me there are probably fair game. Clearly the documentation is wrong since TransactionId could never be {{nullptr}}. I would suggest you put together an RFC outlining what changes you think should be made. Anything short of changing exported method signatures should be fair game. If changes to the API are required to make it function I wouldn't oppose those changes either, they would just need to be introduced carefully. 

> Issues with TrasactionIds managed by CacheTransactionManager in C++ native client
> ---------------------------------------------------------------------------------
>
>                 Key: GEODE-7573
>                 URL: https://issues.apache.org/jira/browse/GEODE-7573
>             Project: Geode
>          Issue Type: Bug
>          Components: native client
>            Reporter: Alberto Gomez
>            Assignee: Alberto Gomez
>            Priority: Major
>
> There are several problems related to the TransactionIds managed by the CacheTransactionManager class:
>  
> On the one hand, according to the documentation, the CacheTransactionManager::getTransactionId() returns null if no transaction is associated to the thread but according to the signature of the method the object returned is of type TransactionId&. Therefore, there is no possibility to return null. The same applies to the CacheTransactionManager::getTransactionId() method.
>  
> If we go to the implementation classes, the following is observed:
> If CacheTransactionManagerImpl::suspend() is invoked and there is no transaction in progress a TransactionException is thrown. This must be documented instead of the current information that states that a null pointer is returned.
> If CacheTransactionManagerImpl::getTransactionId() is invoked and there is no transaction in progress what we get is a segmentation fault because the TXState that should provide the TransactionId object is null. In my opinion the code should be changed to throw an exception just as it is done when suspend() is invoked and there is no transaction in progress.
>  
> On the other hand, once a transaction has been commited, a valid TransactionId reference returned previously by either suspend() or getTransactionId() becomes invalid because the commit of the transaction deletes the object (which is stored in the TXState that is destroyed at commit).
> Subsequent uses of the TransacionId once the transaction is commited like it is done in the testThinClientTransactionsWithSticky integration test (for example calling exists() or resume()) would access freed memory.
> Unawareness of this may cause unexpected behavior in the client code and should be avoided. Two alternatives are proposed:
>  * Document in the C++ API that TransactionId references returned by CacheTransactionManager should not be used after the transaction is commited.
>  * Change the type of object returned/managed to a TransactionId shared pointer.
> The problem with the second approach is that it involves a change in the API so the first alternative is the recommended one.
>  
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)