Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setAutoCommit method to the Transaction class #1459

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Add setAutoCommit method to the Transaction class #1459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bff788e
Add setAutoCommit method to the Transaction class
an-tao Dec 20, 2022
589e573
Lint
an-tao Dec 21, 2022
0cee173
Lint
an-tao Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion orm_lib/inc/drogon/orm/DbClient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,10 +362,16 @@ class Transaction : public DbClient
{
public:
virtual void rollback() = 0;
// virtual void commit() = 0;
virtual void commit(std::function<void(bool)> commitCallback) = 0;
virtual void setCommitCallback(
const std::function<void(bool)> &commitCallback) = 0;

/**
* @pref auto-commit or auto-rollback when the transaction object is
* destroyed; the default behavior is to commit.
*/
virtual void setAutoCommit(bool) = 0;

void closeAll() override
{
}
Expand Down
84 changes: 73 additions & 11 deletions orm_lib/src/TransactionImpl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,26 @@ TransactionImpl::~TransactionImpl()
auto loop = connectionPtr_->loop();
loop->queueInLoop([conn = connectionPtr_,
ucb = std::move(usedUpCallback_),
commitCb = std::move(commitCallback_)]() {
commitCb = std::move(commitCallback_),
autoCommit = autoCommit_]() {
Comment on lines +44 to +45
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UB if the user manually commits then auto commit happens. Could the following happen?

auto t = db->newTransactionCoro();
...
t->commit();
// now `t` destructs. Calls auto commit. We try to move an already moved variable.

conn->setIdleCallback([ucb = std::move(ucb)]() {
if (ucb)
ucb();
});
conn->execSql(
"commit",
autoCommit ? "commit" : "rollback",
0,
{},
{},
{},
[commitCb](const Result &) {
[commitCb, autoCommit](const Result &) {
LOG_TRACE << "Transaction committed!";
if (commitCb)
if (autoCommit && commitCb)
{
commitCb(true);
}
Copy link
Member

@hwc0919 hwc0919 Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be commitCb(autoCommit) ?

same in execption callback

I prefer not using this commitCb. It's confusing.

},
[commitCb](const std::exception_ptr &ePtr) {
[commitCb, autoCommit](const std::exception_ptr &ePtr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the last time we use commitCb. We can move it instead of making a copy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the arguments evaluated in left-to-right order? If not, we can not use move.

try
{
std::rethrow_exception(ePtr);
Expand All @@ -68,7 +69,7 @@ TransactionImpl::~TransactionImpl()
{
LOG_ERROR << "Transaction submission failed:"
<< e.base().what();
if (commitCb)
if (autoCommit && commitCb)
{
commitCb(false);
}
Expand Down Expand Up @@ -150,6 +151,69 @@ void TransactionImpl::execSqlInLoop(
}
}

void TransactionImpl::commit(std::function<void(bool)> callback)
{
auto thisPtr = shared_from_this();
if (callback)
{
commitCallback_ = std::move(callback);
}
loop_->runInLoop([thisPtr]() {
if (thisPtr->isCommitedOrRolledback_)
return;
if (thisPtr->isWorking_)
{
// push sql cmd to buffer;
auto cmdPtr = std::make_shared<SqlCmd>();
cmdPtr->sql_ = "commit";
cmdPtr->parametersNumber_ = 0;
cmdPtr->callback_ = [thisPtr](const Result &) {
LOG_DEBUG << "Transaction commit!";
thisPtr->isCommitedOrRolledback_ = true;
if (thisPtr->commitCallback_)
{
thisPtr->commitCallback_(true);
}
};
cmdPtr->exceptionCallback_ = [thisPtr](const std::exception_ptr &) {
thisPtr->isCommitedOrRolledback_ = true;
if (thisPtr->commitCallback_)
{
thisPtr->commitCallback_(false);
}
LOG_ERROR << "Transaction commit error";
};
cmdPtr->isRollbackOrCommitCmd_ = true;
thisPtr->sqlCmdBuffer_.push_back(std::move(cmdPtr));
return;
}
thisPtr->isWorking_ = true;
thisPtr->thisPtr_ = thisPtr;
thisPtr->connectionPtr_->execSql(
"commit",
0,
{},
{},
{},
[thisPtr](const Result &) {
LOG_TRACE << "Transaction commit!";
thisPtr->isCommitedOrRolledback_ = true;
if (thisPtr->commitCallback_)
{
thisPtr->commitCallback_(true);
}
},
[thisPtr](const std::exception_ptr &) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the last place we used thisPtr. Can move move it to avid a copy?

LOG_ERROR << "Transaction commit error";
thisPtr->isCommitedOrRolledback_ = true;
if (thisPtr->commitCallback_)
{
thisPtr->commitCallback_(false);
}
});
});
}

void TransactionImpl::rollback()
{
auto thisPtr = shared_from_this();
Expand All @@ -172,7 +236,7 @@ void TransactionImpl::rollback()
thisPtr->isCommitedOrRolledback_ = true;
LOG_ERROR << "Transaction roll back error";
};
cmdPtr->isRollbackCmd_ = true;
cmdPtr->isRollbackOrCommitCmd_ = true;
// Rollback cmd should be executed firstly, so we push it in front
// of the list
thisPtr->sqlCmdBuffer_.push_front(std::move(cmdPtr));
Expand All @@ -189,10 +253,8 @@ void TransactionImpl::rollback()
[thisPtr](const Result &) {
LOG_TRACE << "Transaction roll back!";
thisPtr->isCommitedOrRolledback_ = true;
// clearupCb();
},
[thisPtr](const std::exception_ptr &) {
// clearupCb();
LOG_ERROR << "Transaction roll back error";
thisPtr->isCommitedOrRolledback_ = true;
});
Expand Down Expand Up @@ -220,15 +282,15 @@ void TransactionImpl::execNewTask()
std::move(cmd->formats_),
[callback = std::move(cmd->callback_), cmd, thisPtr](
const Result &r) {
if (cmd->isRollbackCmd_)
if (cmd->isRollbackOrCommitCmd_)
{
thisPtr->isCommitedOrRolledback_ = true;
}
if (callback)
callback(r);
},
[cmd, thisPtr](const std::exception_ptr &ePtr) {
if (!cmd->isRollbackCmd_)
if (!cmd->isRollbackOrCommitCmd_)
thisPtr->rollback();
else
{
Expand Down
10 changes: 9 additions & 1 deletion orm_lib/src/TransactionImpl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,16 @@ class TransactionImpl : public Transaction,
timeout_ = timeout;
}

void setAutoCommit(bool autoCommit) override
{
autoCommit_ = autoCommit;
}

void commit(std::function<void(bool)> callback) override;

private:
DbConnectionPtr connectionPtr_;
bool autoCommit_{true};

void execSql(const char *sql,
size_t sqlLength,
Expand Down Expand Up @@ -139,7 +147,7 @@ class TransactionImpl : public Transaction,
std::vector<int> formats_;
QueryCallback callback_;
ExceptPtrCallback exceptionCallback_;
bool isRollbackCmd_{false};
bool isRollbackOrCommitCmd_{false};
std::shared_ptr<TransactionImpl> thisPtr_;
};

Expand Down