add Support for custom Request Object

[phermsdorf]

Hi,

to be able to do custom error handling on server side and use custom request objects it would be great if TCPSocketServerContainer could implement ISharedObjectContainer so that it works the ecf generic provider.

how it worked with the ecf generic provider: we registered a custom IHostContainerSelector service implementation, which registered a custom RegistrySharedObject the code:

@Component(service = IHostContainerSelector.class)
public class AuthHostContainerSelector extends HostContainerSelector {

    public AuthHostContainerSelector() {
        // set the auto create flag to true, so that if the container instance
        // with type specified via SERVICE_EXPORTED_CONFIG
        // does not already exist, then it will be automatically created
        super(null, true);
    }

    @Override
    protected IContainer createContainer(final ServiceReference serviceReference, final Map<String, Object> properties,
            final ContainerTypeDescription containerTypeDescription, final String[] intents)
            throws SelectContainerException {
        final Object object = super.createContainer(serviceReference,
                properties, containerTypeDescription,
                intents);
        final ISharedObjectContainer container = (ISharedObjectContainer) object;

        final ISharedObjectManager manager = container.getSharedObjectManager();

        // Remove any existing registry shared object
        final ID adapterID = IDFactory.getDefault().createStringID(IRemoteServiceContainerAdapter.class.getName());
        manager.removeSharedObject(adapterID);
        try {
            manager.addSharedObject(adapterID, new P4RegistrySharedObjectServer(), null);
        } catch (final SharedObjectAddException e) {
            throw new P4ServerException(e);
        }
        return container;
    }
}

the custom P4RegistrySharedObjectServer did override executeRequest and sendCallResponse to do custom error handling for the Response object. Additionally use a extended Request object to transport additional metadata with the call.

Do you think it could work this way again? How to continue from here?

Thanks for looking into the issue.

Bye Peter

[scottslewis]

Hi Peter,

the custom P4RegistrySharedObjectServer did override executeRequest and sendCallResponse to do custom error handling >for the Response object. Additionally use a extended Request object to transport additional metadata with the call.

Do you think it could work this way again?

It wouldn't be trivial to support the impl of ISharedObjectManager nor RegistrySharedObject in the tcp distribution provider. The reason is that the shared object model has a lot of extra functionality (a group of processes each with a shared object replica + a group membership management/failure detection, etc).

However, it should be pretty easy to support your actual needs (override executeRequest and sendCallResponse/Response and client Request object). Since the tcpserverdistribution provider already uses the Request and Response object. All it will take is to refactor this class some :

https://github.com/ECF/tcpsocketdistributionprovider/blob/master/bundles/org.eclipse.ecf.provider.tcpsocket.server/src/org/eclipse/ecf/provider/tcpsocket/server/TCPSocketServerContainer.java#L61 so that you can override/impl your desired methods.

So I think what might be best is if you would identify what methods you implement in RegistrySharedObject and using that I can do the refactor.

[phermsdorf]

Hi Scott,

in our version of the RegistrySharedObject on client side we have overridden protected Request sendCallRequest(final RemoteServiceRegistrationImpl remoteRegistration, final IRemoteCall call) to create our custom Request object for the remote call. (this is "wired" to the provider via a custom ConsumerContainerSelector service)

On server side we did override the following methods in our version of the RegistrySharedObject:

protected void sendCallResponse(final ID responseTarget, final Response response)
protected void executeRequest(final IExecutor executor, final Request request, final ID responseTarget,
            final RemoteServiceRegistrationImpl localRegistration, final boolean respond)
protected void logRemoteCallException(final String message, final Throwable e)

to log exceptions, serialize exceptions to strings and return that as error response to the client (the client does not have all classes the server has), get the additional metadata from our Request object and to wrap the actual call on the server to get better logging like this:

                if (request instanceof P4Request) {
                    final P4Request p4Request = (P4Request) request;
                    username = p4Request.getUsername();
                    username.ifPresent(service::setCurrentUser);
                    p4Request.getClientId().ifPresent(service::setClientId);
                } else {
                    throw new IllegalStateException("Request from old Client");
                }

                try (MDCCloseable closeable = MDC.putCloseable(LoggerUtil.SESSION_NAME, username.orElse("") + " ")) {
                    result = localRegistration.callService(call);
                }

Is that the information you needed?

Thanks. Bye Peter

[scottslewis]

Hi Peter,

Yes, this is good.  I can't work on this immediately but this gives a pretty good idea.

Scott

On 11/10/2022 4:57 AM, Peter Hermsdorf wrote:

Hi Scott,

in our version of the RegistrySharedObject on client side we have overridden |protected Request sendCallRequest(final RemoteServiceRegistrationImpl remoteRegistration, final IRemoteCall call)| to create our custom Request object for the remote call. (this is "wired" to the provider via a custom ConsumerContainerSelector service)

On server side we did override the following methods in our version of the RegistrySharedObject:

|protected void sendCallResponse(final ID responseTarget, final Response response) protected void executeRequest(final IExecutor executor, final Request request, final ID responseTarget, final RemoteServiceRegistrationImpl localRegistration, final boolean respond) protected void logRemoteCallException(final String message, final Throwable e) |

to log exceptions, serialize exceptions to strings and return that as error response to the client (the client does not have all classes the server has), get the additional metadata from our Request object and to wrap the actual call on the server to get better logging like this:

|if (request instanceof P4Request) { final P4Request p4Request = (P4Request) request; username = p4Request.getUsername(); username.ifPresent(service::setCurrentUser); p4Request.getClientId().ifPresent(service::setClientId); } else { throw new IllegalStateException("Request from old Client"); } try (MDCCloseable closeable = MDC.putCloseable(LoggerUtil.SESSION_NAME, username.orElse("") + " ")) { result = localRegistration.callService(call); } |

Is that the information you needed?

Thanks. Bye Peter

— Reply to this email directly, view it on GitHub https://github.com/ECF/tcpsocketdistributionprovider/issues/3#issuecomment-1310240475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABHB5AR63USYU5YQJSDP3TWHTWKDANCNFSM6AAAAAAR3N4HEQ. You are receiving this because you commented.Message ID: @.***>

[phermsdorf]

Hi @scottslewis ,

i saw you recently had a lot of stuff to do for platform. Could you point me in a direction where to change things so i can try to provide a PR?

Thanks. Bye Peter

[scottslewis]

Hi @phermsdorf.

What I think would be easiest is that you first look at this class (impl of the server socket container):

https://github.com/ECF/tcpsocketdistributionprovider/blob/master/bundles/org.eclipse.ecf.provider.tcpsocket.server/src/org/eclipse/ecf/provider/tcpsocket/server/TCPSocketServerContainer.java

This class implements all of the server/service-side handling of the actual RPC. It's not based upon the RegistrySharedObject, however, so I would first suggest refactoring things (e.g. introduce methods) where you want your methods to be called.

Then I would suggest introducing defining a new interface class (in org.eclipse.ecf.provider.tcpsocket packag in common bundle) that has all the methods (with signatures/return values that are classes that are either in public packages) or with new classes you introduce in the same common bundle package).

Then you can put a public method on TCPSocketServerContainer like:

public void setServerInterceptor(new interface class);

which allows you to hook into the object creation (as you are already doing I think), cast the IContainer to TCPSocketServerContainer and call setServerInterceptor with your impl of new interface class...this so that at remote call time your methods on new interface class will be called and allow you to do what you wish to do/were doing via the RegistrySharedObject overrides.

Another way you can accomplish the same thing would be to define your new service interface, and then register your impl of this interface as an OSGi service using the 'whiteboard' pattern. The rather than you having to hook into the TCPServerSocketContainer creation and defining and calling setServerInterceptor, we can simply have the TCPSocketServerContainer do a lookup in the service registry for all instances of new interface class...and then call any methods from that service at the appropriate time. Less code for you to write than the setServerInterceptor approach above...and no sync code needed, etc. (and you can define your new interface impl via declarative services...which is much easier than doing things in code. Does this make sense? If you are comfortable with it I would suggest this approach over the explicity setServerInterceptor approach (like you are doing now with the generic provider).

[phermsdorf]

That sounds good. At first i would look into be able to replace the creation of the TCPSocketRequest so i can replace that calss with a derived one. Are there any Utility classes to access the OSGi services? Or is it

BundleContext bundleContext = FrameworkUtil.getBundle(getClass()).getBundleContext();
ServiceReference<MyInterface> serviceReference = bundleContext.getServiceReference(MyInterface.class);
MyInterface obj = bundleContext.getService(serviceReference);
-do things-
bundleContext.ungetService(serviceReference);

?

[phermsdorf]

Hi @scottslewis

please see the linked pull request #4 for an initial implementation.

I stumbled upon some points:

• why are all requests done asynchronously? on client side there is a polling for the result with a wait(500). Wouldn't that have a negative performance impact (i.e. response time delay)?
• do you have any code style, formatter and/or cleanup project settings? The projects themselves do not have anything like that. Would it be ok to add these settings? (organize imports, add @override etc.)

Thank you. Bye Peter

[scottslewis]

Hi Peter,

That sounds good. At first i would look into be able to replace the creation of the TCPSocketRequest so i can replace that calss with a derived one. Are there any Utility classes to access the OSGi services? Or is it

BundleContext bundleContext = FrameworkUtil.getBundle(getClass()).getBundleContext();
ServiceReference<MyInterface> serviceReference = bundleContext.getServiceReference(MyInterface.class);
MyInterface obj = bundleContext.getService(serviceReference);
-do things-
bundleContext.ungetService(serviceReference);

?

Code like above for lookup is still supported, but considered obsolete now. The preferred approach for both consuming and defining/implementing services is Declarative Services (part of the OSGi spec).

Speaking only of the consumer-side (service lookup and usage) for the moment, all Declarative Services requires is declaration of a ds 'component' class that uses OSGi DS annotations to dynamically inject service instance(s) into your code at runtime. For example, here is such a class:

https://github.com/eclipse/ecf/blob/master/examples/bundles/com.mycorp.examples.timeservice.consumer.ds.async/src/com/mycorp/examples/timeservice/consumer/ds/async/TimeServiceComponentAsync.java

In this code you will see two annotations:

line 23: @Component(immediate=true) line 24: public class TimeServiceComponentAsync {

This annotation says to DS that this is a component (the immediate=true probably won't be needed for yours).

The critical annotation is:

26: @Reference(cardinality=ReferenceCardinality.AT_LEAST_ONE,policy=ReferencePolicy.DYNAMIC) 27: void bindTimeService(ITimeServiceAsync timeService) {

This annotation tells Declarative Services to inject an instance of ITimeServiceAsync by dynamically calling the bindTimeService method to bind it to this component, and unbindTimeService to unbind it to this component.

You can ignore what's in the parens in both the Component and Reference annotations for the moment and actually this can be simplified even further...e.g.

import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference;

@Component public class MyConsumerOfMyService {

@Reference
private volatile MyService service;

@Activated
public void activated() {
      // Can call service here!
      this.service.callMethod();
}

}

These annotations will result (at runtime) in:

When an instance of MyService becomes available (through registration of the service in service registry...also can be done by DS), then DS will inject the MyService into the service member variable above, and only then call the activated method. DS handles all the dynamics/timing of when these methods can be safely called/injected into your consumer code.

There is a lot more to DS...even just on the consumer side (e.g. the immediate=true for the component and a number of options for the Reference annotation).

But I'm going to suggest that we do the following to work together on this:

You declare a new interface that allows server-side customization of the Request object received from a client ...eg. in org.eclipse.ecf.socket.common bundle in package org.eclipse.ecf.socket.common

public interface TCPSocketRequestCustomizer { public TCPSocketRequest customizeTCPSocketRequest(TCPSocketRequest request); }

and perhaps also

public interface TCPSocketResponseCustomizer { public TCPSocketResponse customizeTCPSocketResponse(TCPSocketResponse response); }

Then create an implementation class in server bundle that implements one or both of these interfaces....e.g. TCPSocketRPCCustomizer implements TCPSocketRequestCustomizer, TCPSocketResponseCustomizer ...impl....

Then create (not as service for the moment) in org.eclipse.ecf.provider.server.TCPSocketServerContainer and use your class to test to see if it's doing as it should, when it should (i.e when during the RPC).

Then when the TCPSocketRequestCustomizer and TCPSocketResponseCustomizer are done/ready I'll add the code to the TCPSocketServerContainer to use ds to use any/all impls of the customizer types.

Then you can you can simply take your test implementation class TCPSocketRPCCustomizer and turn it into DS component in one of your systems bundles (new or old), and then when run in your environment the provider will find your impls registered as services...use them to customize the Request and Response object (and/or call other methods you define). I can/will describe how to use DS to register a service in the service registry. It's actually easier than shown above for a consumer.

How does this sound? Let me know if something isn't clear, but I would start with introducing service interfaces in org.eclipse.ecf.provider.tcpsocket.common.

[scottslewis]

Hi Peter.

I see you've given a pr #4 already ...so forget most of what I said in previous comment. I've approved and merged the pr, and will look at/understand and likely do some refacotring as well as simplification by using DS for service injection. I'll post here this weekend after I've had a chance to look at things and work with them a little bit.

Hi @scottslewis

please see the linked pull request #4 for an initial implementation.

I stumbled upon some points:

* why are all requests done asynchronously? on client side there is a polling for the result with a wait(500). Wouldn't that have a negative performance impact (i.e. response time delay)?

This code isn't for asynchronous requests...it's inside a loop that checks if a the request is done (e.g. r.isDone()) and if it's not done simply waits half a second (500ms) until checking again...and will fail/throw exception if it has to wait > timeout without a response (r.isDone() returns true means a response received).

* do you have any code style, formatter and/or cleanup project settings? The projects themselves do not have anything like that. Would it be ok to add these settings? (organize imports, add @OverRide etc.)

Sure feel free to add settings. You can get default settings from this project (we got setting from Eclipse/Equinox defaults originally):

https://github.com/eclipse/ecf/tree/master/framework/bundles/org.eclipse.ecf/.settings