#1 2016-03-24 12:24:55

oz
Member
Registered: 2015-09-02
Posts: 95

Disabling/switching interface implementations at runtime

Hi Arnaud,

i'm in need of being able to switch/disable service interface implementations without the need to restart the mORMot server. That's why I need to have counterparts for TIntefaceResolver.InjectResolver() and TSQLRestServer.ServiceDefine(). Those two methods would allow to replace/disable interface implementations at runtime.

My DeleteResolver implementation which seems to do it's job:

procedure TInterfaceResolverInjected.DeleteResolver(aResolver: TInterfaceResolver);
var
  ix: integer;
begin
  ix:=ObjArrayFind(fResolvers,aResolver);
  if ix>0 then
    ObjArrayDelete(fResolvers,ix);
end;

But I don't know how to implement TSQLRestServer.ServiceUnDefine without diving deep into the source. Could you provide some help please?

Kind regards,
oz.

Offline

#2 2016-06-21 11:11:08

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

Hi Arnaud,

I forgot about this topic, but I've implemented it successfully some time ago. Could you please review the code and incorporate it into trunk!?

Following changes have to be applied to "mORMot.pas":

1. New public procedure "SeviceUndefine" in TSQLRestServer

procedure TSQLRestServer.ServiceUndefine(const aInterfaces: array of TGUID);
begin
  (ServiceContainer as TServiceContainerServer).DeleteInterface(TInterfaceFactory.GUID2TypeInfo(aInterfaces));
end;

2. New public procedure "DeleteInterface" in TServiceContainer

procedure TServiceContainer.DeleteInterface(const aInterfaces: array of PTypeInfo);
var
  iInterfaces: integer;
  iMethods: integer;
  Idx: integer;
  MethodName: RawUTF8;
  InterfaceName: RawUTF8;
  tmpInterfaceName: RawUTF8;
begin
  for iInterfaces:=Low(aInterfaces) to High(aInterfaces) do begin
    tmpInterfaceName:=aInterfaces[iInterfaces]^.Name;
    if tmpInterfaceName[1] in ['I','i'] then
      Delete(tmpInterfaceName,1,1);
    idx:=fList.IndexOf(tmpInterfaceName);
    if idx>-1 then begin
      fList.Objects[idx].Free;
      fList.Delete(idx);
    end;
    for iMethods:=High(fListInterfaceMethod) downto Low(fListInterfaceMethod) do begin
      Split(fListInterfaceMethod[iMethods].InterfaceDotMethodName,'.',InterfaceName,MethodName);
      if InterfaceName=tmpInterfaceName then
        fListInterfaceMethods.Delete(iMethods);
    end;
  end;
end;

By using those methods one is able to switch Interface-Implementations (and Factories) at runtime.

Kind regards,
oz.

Offline

#3 2016-06-21 12:54:10

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

Oz, is this implementation thread safe? Is this  safe when the server is under load?

Last edited by Leslie7 (2016-06-21 13:25:18)

Offline

#4 2016-06-21 13:51:51

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

Well, imho it's not thread safe because access to TServiceContainer.fList and TServiceContainer.fListInterfaceMethod is done without any kind of locking involved.
I think it's not a good idea to introduce locks for "fList"/"fListInterfaceMethod" access in general. Those fields are accessed in many TServiceContainer methods. Locking every access to those fields could result in performance problems i guess. Maybe it's a good idea to add a lock for the "DeleteInterface" method call itself. This would prevent potential "index out of bounds" exceptions when calling TServiceContainer.DeleteInterface() from several thread at once.

  TServiceContainer = class(TInterfaceResolverInjected)
  protected
    fDeleteInterfaceLock: TSynLocker;
  ...
constructor TServiceContainer.Create(aRest: TSQLRest);
begin
  ...
  fDeleteInterfaceLock.Init;
end;
destructor TServiceContainer.Destroy;
var i: integer;
begin
  for i := 0 to fList.Count-1 do
    fList.Objects[i].Free;
  fList.Free;
  fDeleteInterfaceLock.Done;
  inherited;
end;
procedure TServiceContainer.DeleteInterface(const aInterfaces: array of PTypeInfo);
var
  iInterfaces: integer;
  iMethods: integer;
  Idx: integer;
  MethodName: RawUTF8;
  InterfaceName: RawUTF8;
  tmpInterfaceName: RawUTF8;
begin
  fDeleteInterfaceLock.Lock;
  try
    for iInterfaces:=Low(aInterfaces) to High(aInterfaces) do begin
      tmpInterfaceName:=aInterfaces[iInterfaces]^.Name;
      if tmpInterfaceName[1] in ['I','i'] then
        Delete(tmpInterfaceName,1,1);
      idx:=fList.IndexOf(tmpInterfaceName);
      if idx>-1 then begin
        fList.Objects[idx].Free;
        fList.Delete(idx);
      end;
      for iMethods:=High(fListInterfaceMethod) downto Low(fListInterfaceMethod) do begin
        Split(fListInterfaceMethod[iMethods].InterfaceDotMethodName,'.',InterfaceName,MethodName);
        if InterfaceName=tmpInterfaceName then
          fListInterfaceMethods.Delete(iMethods);
      end;
    end;
  finally
    fDeleteInterfaceLock.UnLock;
  end;
end;

I haven't created a testcase yet, but I see no problems working under server load. The worst thing that could happen is that a call to "Service.Resolve(IDomInterfaceToExchange)" would fail if the Interface is being deleted. I haven't checked what happens to currently allocated "IDomInterfaceToExchange" objects/references when freeing the factory itself. But as far as i understand it should have no impact on already assigned objects implementing that particular interface. They should remain assigned until you set them to NIL.

Offline

#5 2016-06-22 04:13:24

Chaa
Member
Registered: 2011-03-26
Posts: 244

Re: Disabling/switching interface implementations at runtime

To be thread-safe without locking, function DeleteInterface can create modified copies of fList and fListInterfaceMethod, and then call InterlockedExchangePointer to replace old lists with new one.

Or may be create copy of TServiceContainer, and replace it.

Last edited by Chaa (2016-06-22 04:16:28)

Offline

#6 2016-06-22 06:56:08

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

Creating copies of those list does not help here. That would be "thread safe" only by the meaning of no access violations happening.
Imagine what happens if 2 threads are calling DeleteInterface at the same time, each one deleting another interface. Each call to DeleteInterface creates its own copy of the lists, deletes the interface and exchanges the pointer to that list. It will happen that one of those DeleteInterface calls would be "lost" for sure because at the time when Thread 2 is creating the private fList copy, Thread 1 hasn't exchanged the fList pointer yet.

So, if the goal is to make this function thread safe then there is no other way then locking.

I'll write down some more thoughts/advices about that whole thing later...

Offline

#7 2016-06-22 07:48:50

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

Chaa wrote:

To be thread-safe without locking, function DeleteInterface can create modified copies of fList and fListInterfaceMethod, and then call InterlockedExchangePointer to replace old lists with new one.

Or may be create copy of TServiceContainer, and replace it.

This may still cause problems if any code is using the original object at the time of the exchange. It could work if all the routines accessing  the object  do so by assigning it to a local variable which is the only point of access from there. But freeing the object when not needed any more could be tricky in this case.  Using interfaces (which are reference counted)  should be the solution. It seems to me that thread safety  cannot be accomplished without AB having to change some code.

Last edited by Leslie7 (2016-06-22 08:03:00)

Offline

#8 2016-06-22 08:00:28

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

oz wrote:

Creating copies of those list does not help here. That would be "thread safe" only by the meaning of no access violations happening.
Imagine what happens if 2 threads are calling DeleteInterface at the same time, each one deleting another interface. Each call to DeleteInterface creates its own copy of the lists, deletes the interface and exchanges the pointer to that list. It will happen that one of those DeleteInterface calls would be "lost" for sure because at the time when Thread 2 is creating the private fList copy, Thread 1 hasn't exchanged the fList pointer yet.

So, if the goal is to make this function thread safe then there is no other way then locking.

I'll write down some more thoughts/advices about that whole thing later...


Interlocked commands allow you to check if the original value is at place at the time of the exchange.   If not you can restart  the process  in a loop until your modified list can be safely exchanged. Checking for time out in the loop is  most likely not necessary for normal use case, but probably better to include because of possible stress tests.

Last edited by Leslie7 (2016-06-22 13:08:38)

Offline

#9 2016-06-22 16:26:30

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

Leslie7 wrote:

Interlocked commands allow you to check if the original value is at place at the time of the exchange.   If not you can restart  the process  in a loop until your modified list can be safely exchanged. Checking for time out in the loop is  most likely not necessary for normal use case, but probably better to include because of possible stress tests.

I know about Interlocked*** commands, but that would not help in this situation imho. By calling InterlockedExchangePointer you are exactly doing what the method name tells you: you are exchanging one pointer with another one, in a thread safe, atomic way. And that's the problem. Exchanging the pointer is atomic, but the DeleteInterface/AddInterface method is still a non-atomic operation. If 2 or more threads are calling DeleteInterface() method then you will face exactly the same problems as you do without the InterlockedExchangePointer call. It does not help. InterlockedExchangePointer only ensures that exchanging the fList pointer is an atomic operation, it does not care about the data "behind" that pointer. Various threads would "overwrite" fList again and again without seeing each others changes. Comparing to the old value doesn't help you either.

Offline

#10 2016-06-22 16:53:52

Chaa
Member
Registered: 2011-03-26
Posts: 244

Re: Disabling/switching interface implementations at runtime

I mean, that there is a huge number of calls to find service implementation and some rare calls to DeleteInterface/AddInterface.
So we can serialize calls to DeleteInterface/AddInterface, but use lock-free algorithm to find service implementation.

Last edited by Chaa (2016-06-22 16:54:43)

Offline

#11 2016-06-22 18:35:00

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

In my opinion there is no need to make AddInterface() and DeleteInterface() method thread safe at all. AddInterface() exists for a very long time right now, and there has never been any problem with it.
Calling DeleteInterface() is quite a low-level task, and you should have a good reason to do that. This feature is not intended to be called heavily anywhere in your server sources. The programmer is responsible to ensure that AddInterface() and DeleteInterface() method calls are done in thread safe functions. Personally I use those features only inside a single thread safe singleton class.

Last edited by oz (2016-06-22 18:41:52)

Offline

#12 2016-06-22 19:46:29

ab
Administrator
From: France
Registered: 2010-06-21
Posts: 14,183
Website

Re: Disabling/switching interface implementations at runtime

I agree with oz approach.
This is what we use here indoor...

Offline

#13 2016-06-22 22:29:25

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

oz wrote:

In my opinion there is no need to make AddInterface() and DeleteInterface() method thread safe at all. AddInterface() exists for a very long time right now, and there has never been any problem with it.
Calling DeleteInterface() is quite a low-level task, and you should have a good reason to do that. This feature is not intended to be called heavily anywhere in your server sources. The programmer is responsible to ensure that AddInterface() and DeleteInterface() method calls are done in thread safe functions. Personally I use those features only inside a single thread safe singleton class.

Requirements can be different from yours. Some servers need to be on truly 24/7 and it must be possible to change/extend its functionality without restarting or going offline for maintenance. Which can be achieved via plugins or/and scripting, but this is the point when updating the list of interfaces in a thread safe fashion comes to the picture. Currently a mORMot based server must go offline to safely update the published interfaces.

Offline

#14 2016-06-22 23:08:06

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

oz wrote:
Leslie7 wrote:

Interlocked commands allow you to check if the original value is at place at the time of the exchange.   If not you can restart  the process  in a loop until your modified list can be safely exchanged. Checking for time out in the loop is  most likely not necessary for normal use case, but probably better to include because of possible stress tests.

I know about Interlocked*** commands, but that would not help in this situation imho. By calling InterlockedExchangePointer you are exactly doing what the method name tells you: you are exchanging one pointer with another one, in a thread safe, atomic way. And that's the problem. Exchanging the pointer is atomic, but the DeleteInterface/AddInterface method is still a non-atomic operation. If 2 or more threads are calling DeleteInterface() method then you will face exactly the same problems as you do without the InterlockedExchangePointer call. It does not help. InterlockedExchangePointer only ensures that exchanging the fList pointer is an atomic operation, it does not care about the data "behind" that pointer. Various threads would "overwrite" fList again and again without seeing each others changes. Comparing to the old value doesn't help you either.

It works if it is done the right way. smile

The List fList references is always entirely readonly.

for any change
1. OldList:= fList  // two references  to the same list.
2. OldList is duplicated to NewList // two identical lists
3. changes are applied to NewList
4. if  InterlockedCompareExchangePointer (@fList, @NewList , @OldList) <> @OldList then
    // we know that it has been changed since we started --> Start again with 1.
   else
   // we know that it has not been changed since we started and  InterlockedCompareExchangePointer has already replaced  fList  with NewList  --> SUCCESS

Last edited by Leslie7 (2016-06-22 23:13:56)

Offline

#15 2016-06-23 08:47:34

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

Leslie7 wrote:

It works if it is done the right way. smile

The List fList references is always entirely readonly.

for any change
1. OldList:= fList  // two references  to the same list.
2. OldList is duplicated to NewList // two identical lists
3. changes are applied to NewList
4. if  InterlockedCompareExchangePointer (@fList, @NewList , @OldList) <> @OldList then
    // we know that it has been changed since we started --> Start again with 1.
   else
   // we know that it has not been changed since we started and  InterlockedCompareExchangePointer has already replaced  fList  with NewList  --> SUCCESS

Ok, now I get what you mean. But imho it still does not help here. One problem is that not only fList is involved, fListInterfaceMethod is involved too. So we have to do similiar things for fListInterfaceMethod array --> 2 atomic operations in a non-atomic "parent" method. Having a look at the current implementation it should be no problem to operate on fListInterfaceMethod if another thread is operating on fList currently. Possible problems here are more of an academic kind and should only arise if one is trying to Add/Delete the same interface from several thread at the same time.

Leslie7 wrote:

I mean, that there is a huge number of calls to find service implementation and some rare calls to DeleteInterface/AddInterface.
So we can serialize calls to DeleteInterface/AddInterface, but use lock-free algorithm to find service implementation.

Imho there's no other way then locking all calls accessing fList if we want to make the whole thing really thread safe. Have a look at function TInterfaceResolverInjected.TryResolve(aInterface: PTypeInfo; out Obj): boolean;. There's a GlobalInterfaceResolutionLock involved too when adding/deleting/getting interfaces from global storage. It's always dangerous to make Add/Delete methods thread safe, but not Get methods.
The questions is: is it really time-critical to intruduce a lock for the Resolve() method here?

Offline

#16 2016-06-23 09:47:25

oz
Member
Registered: 2015-09-02
Posts: 95

Re: Disabling/switching interface implementations at runtime

So, to come to a conclusion:
AddInterface() will be left untouched for now, introducing some kind of GlobalInterfaceResolutionLock-like locking has to be discussed.

Arnaud, are you ok with adding following 3 methods to mORMot.pas for a start?

procedure TInterfaceResolverInjected.DeleteResolver(aResolver: TInterfaceResolver);
var
  ix: integer;
begin
  ix:=ObjArrayFind(fResolvers,aResolver);
  if ix>=0 then
    ObjArrayDelete(fResolvers,ix);
end;
procedure TSQLRestServer.ServiceUndefine(const aInterfaces: array of TGUID);
begin
  (ServiceContainer as TServiceContainerServer).DeleteInterface(TInterfaceFactory.GUID2TypeInfo(aInterfaces));
end;
procedure TServiceContainer.DeleteInterface(const aInterfaces: array of PTypeInfo);
var
  iInterfaces: integer;
  iMethods: integer;
  Idx: integer;
  MethodName: RawUTF8;
  InterfaceName: RawUTF8;
  tmpInterfaceName: RawUTF8;
begin
  for iInterfaces:=Low(aInterfaces) to High(aInterfaces) do begin
    tmpInterfaceName:=aInterfaces[iInterfaces]^.Name;
    if tmpInterfaceName[1] in ['I','i'] then
      Delete(tmpInterfaceName,1,1);
    idx:=fList.IndexOf(tmpInterfaceName);
    if idx>-1 then begin
      fList.Objects[idx].Free;
      fList.Delete(idx);
    end;
    for iMethods:=High(fListInterfaceMethod) downto Low(fListInterfaceMethod) do begin
      Split(fListInterfaceMethod[iMethods].InterfaceDotMethodName,'.',InterfaceName,MethodName);
      if InterfaceName=tmpInterfaceName then
        fListInterfaceMethods.Delete(iMethods);
    end;
  end;
end;

Any call to AddInterface()/DeleteInterface() methods has to be done in a thread-safe way, which the programmer is responsible for.

Offline

#17 2016-06-23 11:33:40

Leslie7
Member
Registered: 2015-06-25
Posts: 248

Re: Disabling/switching interface implementations at runtime

oz wrote:

Ok, now I get what you mean. But imho it still does not help here. One problem is that not only fList is involved, fListInterfaceMethod is involved too. So we have to do similiar things for fListInterfaceMethod array --> 2 atomic operations in a non-atomic "parent" method. Having a look at the current implementation it should be no problem to operate on fListInterfaceMethod if another thread is operating on fList currently.

fList  and fListInterfaceMethod  are not independent. They have a one to many relationship which can be exploited. If updated in the right order the data structure they represent remains valid even between the two atomic operations. The trick is that when expanding the list fListInterfaceMethod must be updated first and fList is to be updated last. I had a quick look at he code and it seems the TServiceContainer.AddServiceInternal is where changes could be done. When removing items the update order is reversed, fList  must be updated first. When there are both updates and deletes it is a two phase process. All updates first and then all deletes in the second phase.

oz wrote:

Possible problems here are more of an academic kind and should only arise if one is trying to Add/Delete the same interface from several thread at the same time.

This is easy. Since with my suggestion updates are forced to be serialized it is enough  to check if the item to be added/removed already exists in NewList .

Last edited by Leslie7 (2016-06-23 15:05:07)

Offline

Board footer

Powered by FluxBB