Bugfix for TSQLDBServerHttpApi

itSDS · 2015-12-04 12:55:42

Hi AB,

i testet the Modification with ThreadingMode for nearly 24h and the service works as expected !

constructor TSQLDBServerAbstract.Create(aProperties: TSQLDBConnectionProperties;
  const aDatabaseName, aPort, aUserName,aPassword: RawUTF8; aHttps: boolean;
  aThreadPoolCount: integer; aProtocol: TSQLDBProxyConnectionProtocolClass);
begin
  fProperties := aProperties;
{ itSDS
  if fProperties.InheritsFrom(TSQLDBConnectionPropertiesThreadSafe) then
    TSQLDBConnectionPropertiesThreadSafe(fProperties).ThreadingMode := tmMainConnection;
}
  fDatabaseName := aDatabaseName;
  fPort := aPort;
  fHttps := aHttps;
  fThreadPoolCount := aThreadPoolCount;
  if aProtocol=nil then
    aProtocol := TSQLDBRemoteConnectionProtocol;
  fProtocol := aProtocol.Create(TSynAuthentication.Create(aUserName,aPassword));
end;

it was no easy task to find out that ThreadingMode was changed by activating TSQLDBServerHttpApi.
Can you please check and integrate my modifications. We use the Api with SynDBExplorer to check the Database contents.

Concerning the tmThreadPool mode i made the following discoveries and would like to share it here.

A MySQL Server has a default setting of max 150 parallel open Connections. There is an wait_timeout set which decides when to disconnect sleeping connections.
We start 3 mORMot services on one Server all connecting to the same MySQL Server and all doing a lot of queries. After enabling the tmThreadPool we run in another error : To many Connections ...

We solved this issue by setting
1. max_open Connections to 500
2. wait timeout to 120s
3. Set the TSQLDBZEOSConnectionProperties(FConnection).ConnectionTimeOutMinutes := 1;

As result before MySQL kills a sleeping connection after 120s your ConnectionTimeOutMinutes should free the after 60s outdated Connection.
The max Open Count can be reviewed with mysqladmin -uadmin processlist and then changed

danielkuettner · 2015-12-04 13:44:06

Now we have 3 threads for the same problem, tested with 3 different db-frameworks (Zeos, Firedac, Unidac). Have I forgotten one?

@itSDS
The only reason that the your service now runs without a broken connection is:

TSQLDBZEOSConnectionProperties(FConnection).ConnectionTimeOutMinutes := 1;

The number of connections is not relevant.

Mysql has a session.wait_timout (default 28800 = 8 h). As long as this value is not reached, the connection is ok, the service will run.
After this value (timeout) is reached, the connection will be killed from the mysql server instance and the client (service) connection is dead, exceptions will happen.

Over a year ago we had a thread related to this issue for Firebird. TSQLDBZEOSConnectionProperties.ConnectionTimeOutMinutes was our goal.
(http://synopse.info/forum/viewtopic.php?id=1980)

I think it was a good solution. I had never such issues till today.

AB thanks for your great framework!

itSDS · 2015-12-04 16:49:10

@Daniel - i really don't know what your post has to do with my modification concerning the ThreadingMode

The Problem was that mORMot - which is in fact a great framework - used only one Thread for all Connections. Which does not work and results in all the Problems i had.

By Changing 2 lines of code as stated above (or not using TSQLDBServerHttpApi it is solved)

The ConnectionTimeout is very useful to hold the number of connections under Control.

danielkuettner · 2015-12-04 17:19:34

itSDS wrote:

The Problem was that mORMot - which is in fact a great framework - used only one Thread for all Connections.

One Thread for all Connections? What should that be? Multi threads and one connection, that could make sense. But why should I need a single thread with many connections?

But in case it would be useful,

in mORMot you have the choice between one thread for all db activities or multi threads. You have also the choice between one connection or multi connections.

But however your choice will be, it has nothing to do with connection aborts!

itSDS wrote:

The ConnectionTimeout is very useful to hold the number of connections under Control.

Yes, it is very useful, but never ever for the number of connections, but for clearing connections after a time.

If you are agree, we close this thread, because we don't have an mORMot bug here in this case.

itSDS · 2015-12-04 17:30:21

Indeed it was a bug in the framework i commented out in the code sample above. Arnaud set the ThreadingMode from tmThreadPool to tmMainConnection in the constructor... that was the error.

Pls read what i write

ab · 2015-12-04 19:14:46

So the default threading model of SynDBRemote.pas is eventually to be tuned.

I guess the issue comes from the fact that you defined a thread pool, which is not working well with MySQL client.
(note that this was documented as such)
If you simply comment the lines in TSQLDBServerAbstract.Create, the threading mode is set to tmMainConnection, especially for proper transactional support.
Each remote HTTP request is run in its own thread, so there are potential issues, e.g. if you use a transaction.

So the default setting would be highly dependent on the actual protocol library it is using.
I've added aThreadMode: TSQLDBConnectionPropertiesThreadSafeThreadingMode optional parameter to TSQLDBServerAbstract.Create constructors - set to tmMainConnection by default. For tmMainConnection, it would now use a critical section for safe sharing of the single connection.

See http://synopse.info/fossil/info/a955cc28c6

Please try with this lock enabled.
And, it would let tweak the settings to tmThreadPool, if it is not enough in your case.
Note that removing the thread pool would not increase the performance, and enhance security.

itSDS · 2015-12-05 11:21:53

Hi Arnaud, thank you for your answer, and your Modification.

The topic is very complicated cause we deal with 3 interfaces parallel (MVC/ORM and DBRemote) and it is not easy for me to explain in english but i understand what you mean.

I guess the issue comes from the fact that you defined a thread pool, which is not working well with MySQL client.
(note that this was documented as such)

Thats not exactly right. TSQLDBZEOSConnectionProperties,TSQLDBUniDACConnectionProperties and TSQLDBFireDACConnectionProperties are derived from TSQLDBConnectionPropertiesThreadSafe setting ThreadingMode as required to tmThreadPool.

look at:

function TSQLDBConnectionPropertiesThreadSafe.ThreadSafeConnection: TSQLDBConnection;
var i: integer;
begin
  case fThreadingMode of
  tmThreadPool: begin
    EnterCriticalSection(fConnectionCS);
    try
      i := CurrentThreadConnectionIndex;
      if i>=0 then begin
        result := fConnectionPool.List[i];
        if result.IsOutdated then
          fConnectionPool.Delete(i) else // release outdated connection
          exit;
      end;
      result := NewConnection;
      (result as TSQLDBConnectionThreadSafe).fThreadID := GetCurrentThreadId;
      fLatestConnectionRetrievedInPool := fConnectionPool.Add(result)
    finally
      LeaveCriticalSection(fConnectionCS);
     end;
  end;
  tmMainConnection:
    result := inherited GetMainConnection;
  else
    result := nil;
  end;
end;

!! here lies the cause for the Exceptions !!
Setting ThreadingMode to tmMainConnection - as you did in TSQLDBServerAbstract.Create - is a global setting ! Every call to ThreadSafeConnection will get the same (GetMainConnection) now. But the MVC and ORM Threads nead a own (NewConnection) not the MainConnection.

If you simply comment the lines in TSQLDBServerAbstract.Create, the threading mode is set to tmMainConnection, especially for proper transactional support.
Each remote HTTP request is run in its own thread, so there are potential issues, e.g. if you use a transaction.

With my change (which was a simple Workaround) every ThreadSafeConnection gives you a "NewConnection" which is not right if you want to use transactions with DBRemote. Here you are right, and i am with you !

I think SynDB / SynDBRemote is a perfect feature. But to work parallel and with transaction with MVC/ORM and (MySQL,..) every SynDBRemote has to get its own "NewConnection" for the lifetime of the Remote Connection. It is not the right way to switch global to use one "MainConnection"

For my case i use SynDBRemote only to select or update simple Queries which would work with my WorkAround and without transactions. For bigger things i use MySQL Workbench.
Feel free to enhance SynDBRemote if you like

I like your great Framework

danielkuettner · 2015-12-05 11:26:22

I think I've found a little issue why ConnectionTimeOutMinutes don't work as expected in case of using a tmThreadpool instead of tmMainConnection.

If you have more dbconnections the db-server check for all the timeout. But in our service not all must be used.

In SynDB line 4117:

 if (fLastAccessTicks<>0) and
     (Ticks-fLastAccessTicks>fProperties.fConnectionTimeOutTicks) then
    result := true else

The check will not work if fLastAccessTicks = 0 (a "sleeping" connection in the pool).

That means although the fConnectionTimeOutTicks > Ticks-fLastAccessTicks, result is false. But the db-server has already cleared the connection, because wait_timeout was reached.

If you change it to:

if fLastAccessTicks = 0 then
    fLastAccessTicks:= Ticks;

if  (Ticks-fLastAccessTicks>fProperties.fConnectionTimeOutTicks) then
    result := true else

I works, but in my tests it comes an other exception, because of the deletion of a connection while executing a query. That's another issue I don't have a solution for.
The EZSQLException was: "Lost connection to Mysql server during query".

EgonHugeist · 2015-12-05 12:34:50

@AB

i agree with Daniel.

In addition consider a connection should never expire (and erased on server side) between TimeOutMinutes range if the connection was bussy.
From my POV a Stmt should allways reset the fProperties.fConnectionTimeOutTicks if a prepare/executeprepared was called. Because the server has activity and there is no reason to close the connection.

Daniel's second issue looks like the connection is closed while a query was executed. Result is the posted error. If your stmts reset the Connection inactivity tickcount that issue shouldn't happen any more.

What do you think?

itSDS · 2015-12-05 15:27:49

@EgonHugeist you're right a connection should not expire as long as it is in use. But in my opinion there is no Background Thread resetting expired Connections. Connections are only closed(delete) in ThreadSafeConnection.

Most important is to set wait_timeout longer than ConnectionTimeout, On my Server i actually test with wait_timeout = 2Min and ConnectionTimeout = 1Min. And it works ...

BUT today's Morning the MySQL Server has gone away V5.6.17 May be there are some Bugs in it. I had to restart the complete Windows - Server. This is not the Problem of mORMot but MySQL i think. There where 10 SubThread hanging araound as the MySQL Log stated out.

ab · 2015-12-05 15:37:35

@itSDS
Why not just use a dedicated properties class instance, one for each kind of connection?
You are not limited to a single TSQLDBConnectionProperties instance.

@daniel
I've just fixed this code.

@EgonHugeist
I've ensured that overriden TSQLDBStatement.ExecutePrepared methods would reset the associated connection's fLastAccessTicks, as you propose.
This should indeed reduce the issue of connection release during statement execution.

See http://synopse.info/fossil/info/924faf462b

Does it feel right to you all?

itSDS · 2015-12-05 15:38:58

What i think about is how many connection does one mORMot service create.
I can see no limits.
What i see is that a ThreadID is assigned to a connection.
Is this Connection closed if Thread is terminated ?
What happens if 1000 Thread need 1000 Connections but there are only 150 available ?

itSDS · 2015-12-05 15:42:07

@ab i wrote my last post parallel to the post you wrote

Why not just use a dedicated properties class instance, one for each kind of connection?
You are not limited to a single TSQLDBConnectionProperties instance.

i'll think about it

ab · 2015-12-05 15:42:11

You need a thread pool in all cases. You should never have to run 1000 threads creation.
For instance, the HTTP server has its own thread pool.

The connection is expected to be closed by the thread itself, explicitly, when it is terminated.
It is up to the thread to release the resources it uses.
BTW there is no standard way of running some code when any TThread is terminated, under Delphi (there is such a method in FPC).

itSDS · 2015-12-05 16:08:41

I tested now with 2 ConnectionProperties - one for the MVC/ORM Stuff and one for RemoteDB (here ThreadingMode set to default tmMainConnection)

It Works ! It is also the perfect way to deal with Transaction in DB-Remote.
You should write it in the Handbook

danielkuettner · 2015-12-05 16:59:38

@AB
Thanks for your quick fix and reply. I've still an issue, but if I debug longer, my wife will leave me... (sorry for this secrets)

Perhaps I've made a mistake and you see it shortly, here are my (perhaps stupid) little test for it:

Testprops := TMySQLDBZEOSConnectionProperties.Create
    (TSQLDBZEOSConnectionProperties.URI(dMysql, '192.168.1.10:3306',
    'libmysql.dll', false), 'phpgroupware', 'username', 'password');
  TestProps.Execute('select sleep(20)', []);

  TThread.CreateAnonymousThread(procedure
    var
      r: ISQLDBRows;
      s: RawUTF8;
      i, j: Integer;
    begin
     for j:= 1 to 3 do begin
       for i:= 1 to 5 do begin
         r:= TestProps.Execute('select sleep(2)', []);
         while r.Step do begin
           s:= r.ColumnUTF8(0);
         end;
         r:= nil;
         EnterCriticalSection(CriticalSection);
         writeLn('Thread 1 ' + ZFastCode.IntToStr(i));
         LeaveCriticalSection(CriticalSection);
       end;
       sleep(1000*20);
     end;
  end).Start;

  TThread.CreateAnonymousThread(procedure
    var
      r: ISQLDBRows;
      s: RawUTF8;
      i, j: Integer;
    begin
     for j:= 1 to 3 do begin
       for i:= 1 to 5 do begin
         r:= TestProps.Execute('select sleep(2)', []);
         while r.Step do begin
           s:= r.ColumnUTF8(0);
         end;
         r:= nil;
         EnterCriticalSection(CriticalSection);
         writeLn('Thread 2 ' + ZFastCode.IntToStr(i));
         LeaveCriticalSection(CriticalSection);
       end;
       sleep(1000*20);
     end;
  end).Start;

  TThread.CreateAnonymousThread(procedure
    var
      r: ISQLDBRows;
      s: RawUTF8;
      i, j: Integer;
    begin
      for j:= 1 to 3 do begin
       for i:= 1 to 5 do begin
         r:= TestProps.Execute('select sleep(2)', []);
         while r.Step do begin
           s:= r.ColumnUTF8(0);
         end;
         r:= nil;
         EnterCriticalSection(CriticalSection);
         writeLn('Thread 3 ' + ZFastCode.IntToStr(i));
         LeaveCriticalSection(CriticalSection);
       end;
       sleep(1000*20);
     end;
  end).Start;

I've set the session timeout in mysql server to 10 (so short to find the error quicker) and ConnectionTimeOutMinutes to 5 sec (shorter than session.wait_timeout in my.cnf).

The first loop in all 3 threads are good.
In the second loop the connection will be deleted and at Result:= NewConnection (line 2684 in SynDB) access violation results.

danielkuettner · 2015-12-06 09:17:44

Now I'd think, I've found the reason for my exceptions:

        //SynDB.pas line 6279
        
        if result.IsOutdated then begin
          fConnectionPool.Delete(i) <-- here comes the exception;  
        end else begin// release outdated connection

The connection seems still in use by another thread. I can't say it better, because the debugging of this is time critical.

The solution were to comment out "fConnectionPool.Delete(i)". Instead fill a new list with "deprecated connections". The connections in this list will be deleted after a time, to ensure, the connections aren't in use anymore.

mORMot Open Source

#1 2015-12-04 12:55:42

Bugfix for TSQLDBServerHttpApi

#2 2015-12-04 13:44:06

Re: Bugfix for TSQLDBServerHttpApi

#3 2015-12-04 16:49:10

Re: Bugfix for TSQLDBServerHttpApi

#4 2015-12-04 17:19:34

Re: Bugfix for TSQLDBServerHttpApi

#5 2015-12-04 17:30:21

Re: Bugfix for TSQLDBServerHttpApi

#6 2015-12-04 19:14:46

Re: Bugfix for TSQLDBServerHttpApi

#7 2015-12-05 11:21:53

Re: Bugfix for TSQLDBServerHttpApi

#8 2015-12-05 11:26:22

Re: Bugfix for TSQLDBServerHttpApi

#9 2015-12-05 12:34:50

Re: Bugfix for TSQLDBServerHttpApi

#10 2015-12-05 15:27:49

Re: Bugfix for TSQLDBServerHttpApi

#11 2015-12-05 15:37:35

Re: Bugfix for TSQLDBServerHttpApi

#12 2015-12-05 15:38:58

Re: Bugfix for TSQLDBServerHttpApi

#13 2015-12-05 15:42:07

Re: Bugfix for TSQLDBServerHttpApi

#14 2015-12-05 15:42:11

Re: Bugfix for TSQLDBServerHttpApi

#15 2015-12-05 16:08:41

Re: Bugfix for TSQLDBServerHttpApi

#16 2015-12-05 16:59:38

Re: Bugfix for TSQLDBServerHttpApi

#17 2015-12-06 09:17:44

Re: Bugfix for TSQLDBServerHttpApi

Board footer