Disabling/switching interface implementations at runtime

oz · 2016-03-24 12:24:55

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.

oz · 2016-06-21 11:11:08

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.

Leslie7 · 2016-06-21 12:54:10

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)

oz · 2016-06-21 13:51:51

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.

Chaa · 2016-06-22 04:13:24

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)

oz · 2016-06-22 06:56:08

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...

Leslie7 · 2016-06-22 07:48:50

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)

Leslie7 · 2016-06-22 08:00:28

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)

oz · 2016-06-22 16:26:30

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.

Chaa · 2016-06-22 16:53:52

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)

oz · 2016-06-22 18:35:00

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)

ab · 2016-06-22 19:46:29

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

Leslie7 · 2016-06-22 22:29:25

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.

Leslie7 · 2016-06-22 23:08:06

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.

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)

oz · 2016-06-23 08:47:34

Leslie7 wrote:

It works if it is done the right way.
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?

oz · 2016-06-23 09:47:25

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.

Leslie7 · 2016-06-23 11:33:40

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)

mORMot Open Source

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

Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

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

Re: Disabling/switching interface implementations at runtime

Board footer