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

["Alberto Gomez (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/GEODE-7573?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Alberto Gomez updated GEODE-7573:
---------------------------------
    Description: 
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.

 

 

  was:
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 true. 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.

 

 


> 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)