Re: [boost] [transaction] New Boost.Transaction library underdiscussion

Subject: Re: [boost] [transaction] New Boost.Transaction library underdiscussion
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2010-01-18 15:52:49

Hi Stefan

thanks for making available your code on the sandbox. This will help a lot to discuss about the different points of views.

----- Original Message ----- 
From: "Stefan Strasser" <strasser_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Monday, January 18, 2010 4:39 PM
Subject: Re: [boost] [transaction] New Boost.Transaction library underdiscussion

> 
> Am Monday 18 January 2010 14:09:11 schrieb vicente.botet:
>> Hi Stefan, Bob,
>>
>> I've created in the sandbox the transaction directory
>> (http://svn.boost.org/svn/boost/sandbox/transaction/) so we can share our
>> work.
> 
> I've uploaded some code that we've discussed and/or I think needs discussion  
> to transaction/dev:
> https://svn.boost.org/svn/boost/sandbox/transaction/dev/
> 
> it is not meant as an initial code base for "transaction", but only for this 
> discussion.

Looking at the code I think that the lazy creation of resource associated to the created transaction introduce more problems that it solves. How can you ensure that each resource has an equivalent stack of local nested transactions if you create them only when the application access a resource on the context of a global transaction?

 
> basic_transaction_manager.hpp: my implementation of the TransactionManager 
> concept

I find extrange the way basic_transaction_manager is made a singleton. 

 static bool has_active(){  return active_; }

 static basic_transaction_manager &active(){
  if(active_) return *active_;
  else persistent::detail::throw_(no_active_transaction_manager());
 }
 void bind(){ active_=this; }
 static void unbind(){ active_=0; }

What can the application do when there is no transaction_manager? Nothing in my opinion, so the system need to ensure this invariant. I would preffer just an instance() static function. If basic_transaction_manager is not able to define this function, we can make basic_transaction_manager a mixin, that will have the final transaction_manager as parameter. 

template<class Final, class Resources,bool Threads=true,bool TThreads=true>
class basic_transaction_manager {
    static Final& instance() { return Final::instance(); }
    ...
}

Another issue with the current interface for the active transaction.

 void bind_transaction(transaction &tx){
  this->active_transaction(&tx,mpl::bool_<Threads>());
 }
 void unbind_transaction(){
  this->active_transaction(0,mpl::bool_<Threads>());
 }
 transaction &active_transaction() const{
  if(transaction *tx=this->active_transaction(mpl::bool_<Threads>())) return *tx;
  else persistent::detail::throw_(no_active_transaction());
 }
 bool has_active_transaction() const{
  return this->active_transaction(mpl::bool_<Threads>()) ? true : false;
 }

As the active transaction can be null the better is that the function return the pointer to the active transaction. I find this prototype simple.

 transaction* active_transaction() const{
  return this->active_transaction(mpl::bool_<Threads>());
 }
 void active_transaction(transaction* tx) const{
  this->active_transaction(tx,mpl::bool_<Threads>());
 }

With this interface the functions are clearer. Instead of

 typename detail::transaction_construct_t begin_transaction(){
  if(this->has_active_transaction()) return typename detail::transaction_construct_t(&this->active_transaction());
  else return typename detail::transaction_construct_t(0);
 }

we can have

 typename detail::transaction_construct_t begin_transaction(){
  return typename detail::transaction_construct_t(this->active_transaction());
 }

We don't need to test. Just do it. Instead of

if(tx.parent) this->bind_transaction(*tx.parent);
else this->unbind_transaction();

by

this->bind_transaction(tx.parent);

 
> basic_transaction.hpp: the transaction scope class, and the atomic{}retry; 
> macros.

I would separate the language-like macros from the basic_transaction class. 
 
> transaction_manager.hpp: the (configurable) default transaction manager
> 
> basic_loc.hpp and loc.hpp: an example of a C++98 pseudo-template-alias. note 
> the anonymous namespace and the conversion operators in loc.hpp. I think this 
> technically violates the One Definition Rule but I don't think this actually 
> causes a problem.

If I have understood it will be only one transaction_manager class for an application. 
Why add the template parameter  TxMgr to all the other classes? In other words, which class other than transaction_manager can be a template for the basic_loc class?

template<class T>
class loc : public basic_loc<T,transaction_manager>{...}
template<class T>
class loc2 : public basic_loc<T,???>{...}

Here is how I will implement transaction manager

// transaction_manager.hpp
#ifndef BOOST_TRANSACTION_TRANSACTION_MANAGER_HPP
#define BOOST_TRANSACTION_TRANSACTION_MANAGER_HPP
#include BOOST_TRANSACTION_USER_TRANSACTION_MANAGER_HPP
#endif
 
Of course this impose to define always BOOST_TRANSACTION_USER_TRANSACTION_MANAGER_HPP at compilation time, but avoid the template parameter. Thus, instead of

template<class TxMgr>
class basic_transaction;
typedef basic_transaction<transaction_manager> transaction;

we can have just 

class transaction; // using the single transaction_manager

> exception.hpp: defines isolation_exception, with some stuff to unwind 
> the "nested transaction stack" up to the transaction that caused the 
> isolation exception.
> have a look at this, all of our libraries need an exception that is a request 
> to the user to repeat the transaction, be it because of a MVCC failure or 
> because of a deadlock.

I'll come to this point later.

Regards,
Vicente