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