You are not logged in.
Hi,
I think there is a bug in mORMotSQLite3 TSQLRestServerDB.TransactionBegin.
In that method, whenever inherited TransactionBegin(aTable,SessionID) is called, and the result is true,
that means that TSQLRest.fTransactionActiveSession is set to the SessionID of that transaction.
So the transaction session is successfully allocated to this very transaction.
then there is the following code:
try
DB.TransactionBegin; // will call DB.Lock
except
on ESQLite3Exception do
result := false;
end;
So if DB.TransactionBegin were to throw an exception, the exception is caught returning a False, which indicates that running the transaction failed.
However, returning a False is not enough! Because earlier inherited TransactionBegin(aTable,SessionID) succeeded, TSQLRest.fTransactionActiveSession is still set to SessionID, and is NOT reset.
Meaning that whenever this situation occurs (that DB.TransactionBegin throws an exception), fTransactionActiveSession Remains > 0, meaning that NO other transaction can be start, because
inherited TransactionBegin(aTable,SessionID) (which is the first thing that is called whenever you call TransactionBegin on mORMotSQLite3) will always return false as a result.
It ONLY returns true, when fTransactionActiveSession=0.
So EVERY other TransactionBegin call on mORMotSQLite3 will fail if even once the DB.TransactionBegin call in the highlighted code above fails.
Correct me if I'm wrong but that's my conclusion.
Because if we reach the call DB.TransactionBegin, it means that inherited TransactionBegin(aTable,SessionID), meaning that fTransactionActiveSession and fTransactionTable are both allocated to that very call.
meaning that if this transaction fails, inherited rollback needs to be called to release back fTransactionTable and especially fTransactionActiveSession, because that blocks any other attempt to do a transaction on that database.
This issue can be avoided by doing this:
try
TransactionBegin
...
Commit
finally
Rollback
end;
So by basically moving the TransactionBegin INSIDE the Try finally block, that even if TransactionBegin were to fail, and even if there is no actual transaction active, it still calls Rollback, which on its turn Calls the TSQLRest.Rollback in mORMot.pas, which resets fTransactionActiveSession back to 0.
This is a weird solution and shouldn't be necessary, because whenever something goes wrong in DB.TransactionBegin, it still Calls Execute('ROLLBACK TRANSACTION;'); So Me doing an extra rollback in that scenario shouldn't be necessary. And procedure wise doesn't make sense to call Rollback if BeginTransaction failed. Unless it was meant to be used like that which i doubt.
Also this code is not really safe, If TransactionBegin were to fail for other reasons, unless you always use a different sessionID's for each other transaction call you do, especially when done on other threads.
Nonetheless I think this would be a possible solution that would make more sense:
So the exact same code as above, but changed as following: (Note: this is not necessarily the solution I suggest, is just to give an idea)
try
DB.TransactionBegin; // will call DB.Lock
except
on ESQLite3Exception do
begin
result := false;
try
// resetting fTransactionActive flag
inherited RollBack;
Except
//
end;
end;
end;
Once again, because we know fTransactionActiveSession is allocated to that specific call because of the succeeding inherited call, I think it should be safe to do an inherited Rollback call that releases back fTransactionActiveSession.
Once again, correct me if I'm wrong, or feel free to suggest better solutions if possible.
Now, of course there is a reason why I suggest this fix, because I have an issue as following:
Imagine 2 consecutively running threads (2 clients) that both tries to do a transaction at the same time. Useful to note that I always pass GetCurrentThreadId as SessionID.
Now this is the exact scenario that happens:
> Thread 1: BeginTransaction Enter
> Thread 1: BeginTransaction Exit (success)
> Thread 1: Commit Enter
> Thread 1: Execute('COMMIT TRANSACTION') (NOTE: It might be possible that this line takes longer, or is called after Thread 2 has started, but the important part is that it has proceeded enough so that at the point when Thread 2 calls Execute('ROLLBACK TRANSACTION;'), it throws an exception)
> Thread 1: fTransactionActiveSession := 0
> Thread 2: BeginTransaction Enter (Initial check succeeds, because inherited BeginTransaction returns true because fTransactionActiveSession = 0) (IMPORTANT NOTE!!: At this point Thread 1 has NOT set fTransactionActive back to False yet in SynSQLite3 in TSQLDataBase)
> Thread 2: TSQLDataBase.TransactionBegin ENTER
> Thread 2: TSQLDataBase.TransactionBegin Throws Exception: Because fTransactionActive is still true from Thread 1, so instead this code is called by Thread 2: Execute('ROLLBACK TRANSACTION;'); which throws an exception because there is no Running transaction as Thread 1 already called Execute('COMMIT TRANSACTION')
the exception: ESQLite3Exception {"ErrorCode":1,"SQLite3ErrorCode":2,"Message":"Error SQLITE_ERROR (1) using 3.8.7.4 - 'cannot rollback - no transaction is active' extended_errcode=1"}
> Thread 2: Sets fTransactionActive to False in TSQLDataBase.TransactionBegin
> Thread 2: BeginTransaction Exit (while fTransactionActiveSession is still set to Thread 2)
> Thread 1: Commit Exit. (doesn't matter whether or not Thread 2 or 1 exits first)
So as you can see in that example, fTransactionActive (in SynSQLite3 in TSQLDataBase) is set to false too late after fTransactionActiveSession (in mORMot in TSQLRest) is set to 0 by Thread 1, causing TransactionBegin of Thread 2 to proceed further than it should be, causing it to throw an exception and leaving the transaction locked because it doesn't reset fTransactionActiveSession after an exception was thrown.
fTransactionActive (in SynSQLite3 in TSQLDataBase) is in this case the culprit, but that doesn't mean that TransactionBegin should be less safe, because other code is in the wrong. Nevertheless also it does seem to me like fTransactionActive might not be thread safe, and am not sure if that might be related to this issue.
Hope my explanation makes sense and isn't too complicated, can add more details if necessary. Or explain further.
NOTE: The exact order of the calls between the two threads might not be 100% correct, but it's more or less in that order, hopefully making my issue clear.
Last edited by isa (2019-01-28 14:00:10)
Offline
I just noticed on the definition of TransactionBegin the following comment:
// - must be aborted with Rollback if any SQL statement failed
Which is basically what I end up doing in my "bugfix" proposal.
Reading this comment, how am I supposed to write correctly a proper transaction clause?
As this does not comply with that comment:
TransactionBegin
try
...
Commit
finally
Rollback
end;
Any SQL Exception is caught INSIDE TransactionBegin, so there is no way for us to tell whether or not an exception occurred, and if or not this is an exception as a result of a failed SQL statement.
EDIT: NOTE: I forgot to mention. That in my code whenever TransactionBegin fails, I stop that code, as I don't want to proceed doing actions on the database, if the transaction hasn't started yet, nor do I want to call rollback, in case transactionBegin failed, because another transaction was in progress. So I either need specific details as to why the TransactionBegin Failed, or upon failure, transactionBegin should do a proper fallback depending on why it failed.
Last edited by isa (2019-01-29 09:20:43)
Offline
It took me a while but I see your point, you are correct, there is a chance that TransactionBegin will call Execute('ROLLBACK TRANSACTION;') and throw exception just because fTransactionActive is not actually thread safe.
However, this seems to be accounted for, if you look at TSQLRestServerDB.TransactionBegin :
function TSQLRestServerDB.TransactionBegin(aTable: TSQLRecordClass; SessionID: cardinal=1): boolean;
begin
result := inherited TransactionBegin(aTable,SessionID);
if not result then
exit; // fTransactionActive flag was already set
try
DB.TransactionBegin; // will call DB.Lock
except
on ESQLite3Exception do
result := false;
end;
end;
In your example, DB.TransactionBegin will throw exception and it's caught and result is set to false.
You can inherit from Server classes so you can fine tune this function yourself to return more info if you need.
Personally I think the transaction code is very clever, I like how TransactionActiveSession is actually released at the TSQLRest level before any transaction commit or rollback actually takes place, so that in a multi-thread env, threads don't need to wait until Database execution has already happened but instead are automatically blocked through a critical lock when they try to Commit.
Last edited by pvn0 (2019-01-29 10:25:20)
Offline
It took me a while but I see your point, you are correct, there is a chance that TransactionBegin will call
Execute('ROLLBACK TRANSACTION;')
and throw exception just because fTransactionActive is not actually thread safe.
However, this seems to be accounted for, if you look at TSQLRestServerDB.TransactionBegin :
function TSQLRestServerDB.TransactionBegin(aTable: TSQLRecordClass; SessionID: cardinal=1): boolean; begin result := inherited TransactionBegin(aTable,SessionID); if not result then exit; // fTransactionActive flag was already set try DB.TransactionBegin; // will call DB.Lock except on ESQLite3Exception do result := false; end; end;
In your example, DB.TransactionBegin will throw exception and it's caught and result is set to false.
You can inherit from Server classes so you can fine tune this function yourself to return more info if you need.Personally I think the transaction code is very clever, I like how ActiveTransactionSession is actually released at the TSQLRest level before any transaction commit or rollback actually takes place, so that in a multi-thread env, threads don't need to wait until Database execution has already happened but instead are automatically blocked through a critical lock when they try to Commit.
First of all, thank you for your reply.
I have no problem with how fActiveTransactionSession is released with a commit.
On a commit there are 3 things that needs to be done for another transaction to be able to starts which is in this order:
fActiveTransactionSession := 0; // this is done thread safe
Execute('COMMIT TRANSACTION;'); // This is the actual release on the SQL database
fTransactionActive := False; // This one is set to false at the very last moment in case of a commit, yet on a transaction begin this is the last check that is done before another transaction starts (not necessarily a bad thing), and it seems like it's not thread safe.
Like you mentioned, this is overall good, it works perfectly as intended 99% of the time, if not more. Thing is, I have a rather large mORMot server with lots of users on it. so it can happen like once or twice a week that I have this bug.
I mean it's even stated as comments on the TransactionBegin definition, that a rollback has to be done in case of a sql execution error.
The SQL Execution error does happen in my case because of not being fully thread safe. Not a huge deal, it has proper error handling, so nothing goes wrong, apart from the part that rollback is not called.
And with rollback I mean the rollback method of TSQLRest.RollBack in mORMot.pas, though a full rollback call from TSQLRestServerDB.RollBack should also do the trick of course.
Perhaps this rollback is not done, because not in all cases the SQL Error is due to there not being anything that can be rolled back.
Perhaps Arnaud decided to do this, just to make sure that in case the actual rollback on the SQL failed (meaning that the transaction is still active), he wants to avoid calling TSQLRest.RollBack as it is not a wanted scenario. So basically this not necessarily being a bug but a decision.
As you saw the TransactionBegin does an Execute('ROLLBACK TRANSACTION;') in TSQLDataBase.TransactionBegin in case fTransactionActive is True. If you ask me, If this happens, clearly a rollback is called on SQL level. so it should also be called on mORMot level on TSQLRest. That's the inconsistency here.
So if fTransactionActive is True,
Execute('ROLLBACK TRANSACTION;') is called, which is good
fTransactionActive is set to False, which is also good
But fActiveTransactionSession remains what it is. So in that Rollback scenario that happens inside TransactionBegin, in some way also TSQLRest.RollBack in mORMot.pas has to be called. so that TSQLRest in mORMot.pas and TSQLDataBase in SynSQLite3 are in sync.
And once fActiveTransactionSession remains > 0 while fTransactionActive is set to false, you cannot start a transaction anymore whatsoever unless you do a manual Full Rollback first.
And my issue was even worse. I didn't used to use a separate SessionID's for my transactions until a couple months back. Causing journal files to be created after this bug happened, and every single change on the database that happens outside transactions would be stored in those journals, and I had no manual code to call commit, and I also wasn't sure back then if this transaction bug was caused due to some error in the code, thus me not necessarily wanting to commit but do a rollback instead, but that also meant that I would loose all those other changes stored in those journal files. Causing lots of issues with our users.
Luckily the journal file issues are gone ever since using the thread ID of each separate client connection as the session ID, but still this bug happens from time to time, blocking any other transaction to run from that point on until I restart the running REST server.
So yeah, not certain what possible solution I'd go for this, as I also don't want to possibly introduce other unwanted behavior as a result of my changes and me potentially overlooking thins.
Last edited by isa (2019-01-29 11:01:42)
Offline
OK, I've created a minimal working example to demonstrate that this bug is legit, @isa please confirm this is the same problem you have.
This example creates (n) number of async tasks that start their own transactions, each task also sleeps anywhere from 0-5000 ms before trying to create a transaction to better simulate real life workload. Like @isa explained, eventually the code that isn't thread safe will trip over itself and from that point on , no transactions can be done at all.
source code below, using https://github.com/gabr42/OmniThreadLibrary for threading so get that if you don't have it yet. Compiled and tested with Delphi XE7 64bit.
program Project1;
uses
Vcl.Forms,
Unit1 in 'Unit1.pas' {Form1};
{$R *.res}
begin
Application.Initialize;
Application.MainFormOnTaskbar := True;
Application.CreateForm(TMainForm, MainForm);
Application.Run;
end.
object MainForm: TMainForm
Left = 0
Top = 0
Caption = 'Test'
ClientHeight = 399
ClientWidth = 562
Color = clBtnFace
Font.Charset = DEFAULT_CHARSET
Font.Color = clWindowText
Font.Height = -11
Font.Name = 'Tahoma'
Font.Style = []
OldCreateOrder = False
OnCreate = FormCreate
PixelsPerInch = 96
TextHeight = 13
object lb: TListBox
Left = 0
Top = 64
Width = 562
Height = 335
Align = alBottom
ItemHeight = 13
TabOrder = 0
end
object bRunMany: TButton
Left = 16
Top = 16
Width = 75
Height = 25
Caption = 'Run'
TabOrder = 1
OnClick = bRunManyClick
end
object editMany: TEdit
Left = 104
Top = 18
Width = 89
Height = 21
NumbersOnly = True
TabOrder = 2
Text = '256'
end
object bRunSingle: TButton
Left = 464
Top = 16
Width = 75
Height = 25
Caption = 'Run single'
TabOrder = 3
OnClick = bRunSingleClick
end
end
unit Unit1;
interface
uses
Winapi.Windows,
Winapi.Messages,
System.SysUtils,
System.Variants,
System.Classes,
Vcl.Graphics,
Vcl.Controls,
Vcl.Forms,
Vcl.StdCtrls,
SynCommons,
Synsqlite3Static,
mORMot,
mORMotSQLite3,
OTLComm,
OtlTask,
OtlParallel,
OtlTaskControl;
const
MSG_LOG = WM_APP + 1;
type
TSQLTest = class(TSQLRecord)
private
FA, FB, FC, FD, FE: RawUTF8;
published
property A: RawUTF8 read FA write FA;
property B: RawUTF8 read FB write FB;
property C: RawUTF8 read FC write FC;
property D: RawUTF8 read FD write FD;
property E: RawUTF8 read FE write FE;
end;
TMainForm = class(TForm)
lb: TListBox;
bRunMany: TButton;
editMany: TEdit;
bRunSingle: TButton;
procedure FormCreate(Sender: TObject);
procedure bRunManyClick(Sender: TObject);
procedure bRunSingleClick(Sender: TObject);
private
fModel: TSQLModel;
fRestServer: TSQLRestServerDB;
protected
procedure AddRecords(const task: IOmniTask);
procedure OnTaskMessage(const task: IOmniTaskControl; const msg: TOmniMessage);
procedure OnTaskTerminated(const task: IOmniTaskControl);
end;
var
MainForm: TMainForm;
implementation
{$R *.dfm}
procedure TMainForm.AddRecords(const task: IOmniTask);
var
I: Integer;
R: TSQLTest;
taskID: Cardinal;
lLength: Integer;
lArray: TArray<TSQLTest>;
begin
Randomize;
Sleep(Random(5000));
if task.CancellationToken.IsSignalled then
exit;
taskID := GetCurrentThreadId;
Randomize;
lLength := Random(256);
SetLength(lArray, lLength);
try
for I := 0 to lLength - 1 do
begin
R := TSQLTest.Create;
R.A := SynCommons.StringToUTF8(TGUID.NewGuid.ToString);
R.B := SynCommons.StringToUTF8(TGUID.NewGuid.ToString);
R.C := SynCommons.StringToUTF8(TGUID.NewGuid.ToString);
R.D := SynCommons.StringToUTF8(TGUID.NewGuid.ToString);
R.E := SynCommons.StringToUTF8(TGUID.NewGuid.ToString);
lArray[I] := R;
end;
task.Comm.Send(MSG_LOG, Format('task(%d) starting transaction', [taskID]));
if fRestServer.TransactionBegin(TSQLTest, taskID) then
begin
try
task.Comm.Send(MSG_LOG, Format('task(%d) Transaction started', [taskID]));
// .... modify the database content, raise exceptions on error
for I := 0 to lLength - 1 do
fRestServer.Add(lArray[I], true);
task.Comm.Send(MSG_LOG, Format('task(%d) Trying to Commit ...', [taskID]));
fRestServer.Commit(taskID, true);
task.Comm.Send(MSG_LOG, Format('task(%d) Commit Success!', [taskID]));
except
on E: Exception do
begin
task.Comm.Send(MSG_LOG, Format('task(%d) Performing RollBack, EXCEPTION : %s', [taskID, E.Message]));
fRestServer.RollBack(taskID); // in case of error
end;
end;
end
else
task.Comm.Send(MSG_LOG, Format('task(%d) could not start Transaction', [taskID]));
finally
for I := 0 to lLength - 1 do
lArray[I].Free;
end;
end;
procedure TMainForm.bRunManyClick(Sender: TObject);
var
I: Integer;
begin
for I := 0 to StrToInt(editMany.Text) - 1 do
Parallel.Async(AddRecords, Parallel.TaskConfig.OnMessage(OnTaskMessage).OnTerminated(OnTaskTerminated));
end;
procedure TMainForm.bRunSingleClick(Sender: TObject);
begin
Parallel.Async(AddRecords, Parallel.TaskConfig.OnMessage(OnTaskMessage).OnTerminated(OnTaskTerminated));
end;
procedure TMainForm.FormCreate(Sender: TObject);
begin
fModel := TSQLModel.Create([TSQLTest]);
fRestServer := TSQLRestServerDB.Create(fModel, ExeVersion.ProgramFilePath + 'test.db');
fModel.Owner := fRestServer;
fRestServer.CreateMissingTables;
end;
procedure TMainForm.OnTaskMessage(const task: IOmniTaskControl; const msg: TOmniMessage);
begin
if msg.MsgID = MSG_LOG then
lb.Items.Add(msg.MsgData.AsString);
end;
procedure TMainForm.OnTaskTerminated(const task: IOmniTaskControl);
var
lException: Exception;
begin
if not Application.Terminated then
if Assigned(task.FatalException) then
begin
lException := task.DetachException;
try
lb.Items.Add(lException.Message);
finally
lException.Free;
end;
end;
end;
end.
Last edited by pvn0 (2019-01-30 10:56:10)
Offline
Indeed, this seems exactly like the issue. Especially if between the Commit succes and first Transaction Start failure an exception took place of this type: 'cannot rollback - no transaction is active'.
Thank you very much for your time and effort!
The solution I did was the following, inside the TransactionBegin in mORMotSQLite3.pas:
function TSQLRestServerDB.TransactionBegin(aTable: TSQLRecordClass; SessionID: cardinal=1): boolean;
begin
result := inherited TransactionBegin(aTable,SessionID);
if not result then
exit; // fTransactionActive flag was already set
try
DB.TransactionBegin; // will call DB.Lock
except
on ESQLite3Exception do
begin
result := false;
try
// releasing fTransactionActive flag
inherited RollBack(SessionID);
Except
//
end;
end;
end;
end;
Now this seems a little uglyish to have a try except in a try except, so I also had this as a solution:
function TSQLRestServerDB.TransactionBegin(aTable: TSQLRecordClass; SessionID: cardinal=1): boolean;
begin
result := inherited TransactionBegin(aTable,SessionID);
if not result then
exit; // fTransactionActive flag was already set
try
try
DB.TransactionBegin; // will call DB.Lock
finally
if NOT DB.TransactionActive then
inherited RollBack(SessionID);
end
except
on ESQLite3Exception do
result := false;
end;
end;
Now that 2nd one should also be okay, as the only scenario where DB.TransactionBegin ends with fTransactionActive remaining false, is when the call Execute('ROLLBACK TRANSACTION') throws an exception, which is caught by the try except.
So both solutions should be fine, but I did prefer the 1st solution, because me not being able to fully trust fTransactionActive being thread safe, especially when SessionID's are not unique per transaction.
Once again thank you for your efforts. Really appreciate that.
Since you already have that code, is it also possible for you to test whether or not the issue is actually solved after adding the fix I mentioned to mORMotSQLite3.pas?
Offline
@isa , your 1st solution fixes the problem, nice work! @ab please review this.
Offline
@isa , your 1st solution fixes the problem, nice work! @ab please review this.
Thank you for the the confirmation, and once again thank you for replying to this post and taking your time and effort to look this up.
Really appreciate that!
Thank you.
Offline
Exceptions can be very expensive tho, another approach would be to read/write your own crit.locked boolean value whenever you do start/finish transaction on server side. That way you will always 100% know if transaction is active or not.
Best regards.
Offline
@ab
I have been using my fix for a while now, and my issues are solved.
Could you perhaps look into this? Is there perhaps a reason you don't want to change this? Or do you perhaps have other plans with mORMot, now that you switched to AI development?
Offline
@ab, @isa,
Please see #191 pull request. I've tested and I believe this is best fix.
Thank you for sharing your attempt on fixing it.
However that does not work for my needs.
In your fix you simply check the result of the "DB.TransactionBegin" call alongside the existing result check of "inherited TransactionBegin(aTable,SessionID)"
Problem is that FB.TransactionBegin Doesn't simply return False, but raises an Exception. So your check wouldn't make any difference in my scenario. Still leaving the Transaction at mormot level not rolled back.
Once again my solution was to change the Code in the Except block to:
except
on ESQLite3Exception do
begin
result := false;
try
// releasing fTransactionActive flag
inherited RollBack(SessionID);
Except
//
end;
end;
end;
As I did state above. This works good enough for me. Not saying this is the best solution, but your solution unfortunately doesn't fix my issue.
Last edited by isa (2019-04-26 13:00:40)
Offline
I am happy your solution works for you, however I think you have misread the fix. The reason you get exception in DB.TransactionBegin is because the race condition creates a soft lock always thinking there is an active transaction when in reality there is not and as a result the db sqlite code will throw exception in something like Exception: "no transaction active" when the TransactionBegin tries to rollback a previous transaction that is no longer active.
Either way, I have tested this fix with the sample program I provided in this thread and it solves this problem.
Offline
I am happy your solution works for you, however I think you have misread the fix. The reason you get exception in DB.TransactionBegin is because the race condition creates a soft lock always thinking there is an active transaction when in reality there is not and as a result the db sqlite code will throw exception in something like Exception: "no transaction active" when the TransactionBegin tries to rollback a previous transaction that is no longer active.
Either way, I have tested this fix with the sample program I provided in this thread and it solves this problem.
My bad, indeed you call DB.BeingTransaction BEFORE Inherited transaction.
Thanks for your input.
Offline