Hosted Client

[ReubenBond]

This PR adds seamless support to silos for calling grain methods, using streams, local objects, and any other client functionality from outside of the context of an activation or system target.

The primary scenarios this enables are:

• Efficient co-hosting of HTTP/etc servers (ASP.NET, Orleans Dashboard, gRPC) - no extra serialization/scheduling overhead.
• Ability to make grain calls from injected services, such as IPlacementDirectors, without having to use TaskScheduler capturing tricks.

Since this enables grain calls from non-grain threads, this also adds some guard rails: methods on Grain/Grain<T> are barred from outside of an activation context by virtue of RuntimeContext checks on GrainRuntime and GrainStorageBridge.

CI looks good, VSO looks good, load tests look good.

Reviewed by @dVakulen.

I changed the name from LocalClient/Silo-Local Client to Hosted Client. I think that sounds better, but please let me know if you feel that's an unsuitable name.

How it works

• When accessing runtime from an outside thread for the first time, we instantiate & install IHostedClient into ISiloMessageCenter and IClientObserverRegistrar.
• IHostedClient creates and registers its own ClientId so that it can receive calls.
• Client-bound calls are routed to IHostedClient if the id matches
• IRuntimeClient methods behave differently depending on whether RuntimeContext.Current is null (hosted client) or not (running in activation context). Generally speaking, instead of throwing an exception when the context is null, we handle the operation via the hosted client.
• Grain method calls go through the regular InsideRuntimeClient pathway, but calls to grain observers (aka LocalObject aka InvokableObject) are routed to the IHostedClient implementation, which maintains a thread to schedule Messages which are destined for local objects. This logic was largely extracted from OutsideRuntimeClient and into a shared class, InvokableObjectManager. So this thread is only used for those calls and not for regular calls. This Grain Observer flow is an area to optimize in future (via shared executor service), but is not in need of optimization now: performance for those objects ought to be the same as it is for the external client today.

OLD, busted description (ignore this)

This PR is a work-in-progress of an IClusterClient implementation which communicates directly with a silo in the same process.

The current implementation requires serialization for the Message objects sent between the client and silo. This is because GrainReferences must have their IRuntimeClient property set to the appropriate value (in this case either the LocalClient instance or the InsideRuntimeClient instance).

An alternative implementation might be feasible: instead of providing a completely separate IRuntimeClient implementation we could modify InsideRuntimeClient so that when it's being called from a non-Orleans context (RuntimeContext.Current == null) it defers to the implementations in this PR's LocalClient. As a result, issues such as #3273 and #3256 might not arise. However this also means that it's more difficult to catch bugs where we should be executing within a SystemTarget or Grain but have not correctly set the RuntimeContext. It's not inherently wrong to make grain calls from within a grain via Task.Run(), but that is the primary way we catch things which are bad (eg, modifying grain state). Another way to catch these kinds of bugs is to insert checks on Grain and Grain<T> members: that way, accessing GrainFactory or calling WriteStateAsync() from within Task.Run() would be disallowed (it's permitted today). I think this is a good approach - we can gain some coverage there, even if we're also losing some coverage because of this PR.

@gabikliot & others, do you have thoughts on that previous paragraph?

This ought to help with use cases such as alternative networking layers such as for HTTP & gRPC support.

• Installs a second 'gateway' called ILocalClientGateway into the silo
• Multiple ILocalClient implementations can be created and customized based upon the silo instance (maybe this should be changed to one!)
• Supports grain calls, observers, and streaming, but I have not yet added in-depth tests.
• Still a work in progress

[ReubenBond]

Update: the current revision implements the local client as an automatic option for when calls are made on a non-Orleans thread. This means that Message objects can be passed around in memory without copying and GranReferences do not need to be bound to a different IRuntimeClient when moving between Orleans threads & threadpool threads

[ReubenBond]

If we generally agree on the approach then I'll fix the merge conflicts, improve tests, and move things to a better namespace.

I believe this new approach is superior from an end-user perspective.

• Removed ILocalClientGateway and LocalClientBuilder, it's just ILocalClient now, which is internal. The only way to access the client is via IClusterClient, which can be retrieved from the dependency injection container.
• ILocalClient can no longer be customized and only a single instance exists per silo.
• ILocalClient will install itself upon first use (when a threadpool thread accesses the runtime, see InsideRuntimeClient.ThreadPoolClient).
• New code is minimal: < 200 lines in LocalClient.cs with the majority dedicated to observers / streams (grain extensions).
• Shared code paths for sending and receiving messages, just some minor tweaks on each path.
• Streams work naturally, without modifying any of the streaming code.
• Currently we have the notion of a SystemThread which is a kind of ISchedulingContext. They are not used in our system, but I left SystemThread as being unable to make grain calls. This is easily rectified if desired.

[attilah]

Beside that one comment it looks good to me, I like the simplified functionality, it is something I "envisioned" as "must have" when did the HTTP prototyping.

[attilah]

Is there a reason why it is made public, since everything is internal, it has Dispose, but not IDisposable.

[ReubenBond]

No reason, I should change it. The Dispose should be moved above this class, it's just an illusion

[gabikliot]

@ReubenBond, I need to look more in the code (I can't do it now), but if u do ask my opinion, at this stage, I think I would advise against this approach, due to all the reasons you mentioned yourself. I would stick with strong isolation and faster bugs discovery.
Instead, I would allow clients to be used explicitly in the same process with silo, the same way as u allow multiple clients ( non static client work has been done I assume).

[ReubenBond]

Thanks @gabikliot. Initially I was against it, too, but I convinced myself otherwise. Calling a grain from a non-grain context is not what's unsafe: what's unsafe is mutating grain internals from a non-grain context. It's a useful check, but there are more accurate checks we can make. Eg, we could perform checks whenever accessing Grain/Grain<T>/SystemTarget methods/properties. So if a user calls WriteStateAsync() or accesses State or GrainFactory on the wrong thread/context then we can throw. In my view, those more closely align with what's correct vs what's incorrect.

Today, we wouldn't complain if the user had this in their grain code:

Task.Run(async () =>
{
  this.State.Value++;
  // we will complain if the storage provider is implemented using grains, but not otherwise.
  await this.WriteStateAsync();
});

Of course, we could add those checks anyway - nothing to do with this PR. I'm just saying that IMO we're checking for the wrong thing.

So we should consider whether it's worthwhile to lose that check, gain local client (which fixes our issue last week with non-SystemTarget services making grain calls) and add more rigorous checks on the base classes.

[jdom]

why was the Reset method removed from the interface, forcing a need to downcast?

[ReubenBond]

The interface is for all IRuntimeClient, but Reset(bool) is only implemented in OutsideRuntimeClient - it would throw on InsideRuntimeClient

[ReubenBond]

It might make sense to have some kind of specific ClusterClient or an interface for the external runtime client.

[jdom]

you are using ?. above, so I assume there is a chance for it to be null. If that's the case, then the AsReference call might throw a NullReferenceException

[jdom]

merge conflict

[sergeybykov]

This looks like a clever hack to me. I'm sure we can make it work, but I'm afraid it'll still be a hack. The sheer number of

if (RuntimeContext.Current == null)

in this PR gives me the hacky vibe.

Couldn't we instead make an honest instance/subclass of OutsideRuntimeClient with a special implementation of IMessageCenter injected into it for cheap passing of messages back and forth?

Calling a grain from a non-grain context is not what's unsafe:

While this statement may be true, it seems a bit misleading to me. The goal here is to have an efficient embedded client to be used by transports like HTTP or gRPC. Hence, the question of safety of invoking a grain from a non-grain context I think is orthogonal.

InsideRuntimeClient was made for a different purpose - to facilitate calls made by grains. OTOH, the purpose of OutsideRuntimeClient is exactly what we need here, sans efficiency of local message passing.

Did I miss something?

[ReubenBond]

The issue with having separate inside/outside IRuntimeClient impls is that GrainReferences are bound to a given IRuntimeClient. When a ref moves between threads (eg, from a grain to a client), we need some way of making sure everything continues working. The original method I used for doing that in this PR was essentially to force a deep copy of the entire message - including "immutable" objects such as grain references. That involved serializing with the silo's serializer and deserializing with the client's serializer. Certainly not efficient, and also required 2x assembly loading/processing.

The LocalClient in this PR could me moved behind an interface and the "grain client" could be moved behind the same interface. Then the RuntimeContext.Current check just switches between two implementations of the same interface. That's cleaner in a sense.

InsideRuntimeClient was made for a different purpose - to facilitate calls made by grains. OTOH, the purpose of OutsideRuntimeClient is exactly what we need here, sans efficiency of local message passing.

Did I miss something?

I don't think you're really missing anything, but our takes are a little different. I figured that the current approach was less error prone than the alternatives whilst retaining efficiency. One alternative is to make the IRuntimeClient a thread-local variable, so that users can initialize a thread with a runtime "client.InitializeThread()" or something, or maybe have a using block and an async-local runtime client. My view was that these approaches were messy and error prone and they make it much more difficult to deal with clients calling into multiple clusters (which the current solution supports seamlessly - grains are never re-bound, so they always talk to their originating cluster)

A non-solution (in my view) is to require that all grain referenced be explicitly re-bound before use - that spills too much complexity into user code and is error prone.

The main thing I wanted to achieve agreement on before performing cleanup was the idea of a GrainReference being usable from any thread without modification.

[shlomiw]

Hi, this would be helpful in many scenarios.
In my case I have a common layer between clients and silos and I'd like to simply inject IClusterClient, so it could subscribe to streams and call grains.
This would perfectly fit.
Any plans to push it?
As always - many thanks!

[ivanpaisqcl]

Hi,
Will this feature be merged in? It would help in a few scenarios.

[sergeybykov]

@ivanpaisqcl We haven't decided yet. It's a very powerful feature, and we want to carefully assess the downsides of enabling potentially very error prone cases.

[ivanpaisqcl]

@sergeybykov Is there any timeline to make the first decision. We could really use such a feature, however if it is not going to be anytime soon, we may have to decide on an alternative.

[galvesribeiro]

@ivanpaisqcl I'm on the same boat as you expecting this feature to be released. In meanwhile, I'm creating a client in the same process and making it to connect directly to the localhost instead of using the membership provider until we have this feature merged.

[shlomiw]

@galvesribeiro - I'm doing the same :)

[galvesribeiro]

@shlomiw it is not the ideal since you still have serialization and local networking happening but, at least it is a temporary work around and should not be complicated to move to LocalClient once it is merged.

[ReubenBond]

My intention is to revisit this after we release 2.0

[ReubenBond]

We had a team meeting to reach consensus on this PR.
The result was that the team is ok with the PR but wanted it to be opt-in instead of opt-out. I prefer it be opt-out and not easy to opt out of, but hopefully this is just a transition step until people get comfortable with the idea.

As such, I've added a new extension method to enable this functionality: ISiloHostBuilder.EnableExternalContextCalls().

Let me know if the name makes you feel suitably nauseous (it's supposed to) or if you want it changed to, eg, EnableHostedClient(), EnableCallsFromExternalContext(), etc.

[galvesribeiro]

EnableExternalContextCalls will just enable a flag?

If that is the case, how do we get an IClusterClient with the local implementation rather then have to specify membership, address, etc?

I'm not following the decision (too confuse)...

IMHO, a clear way to be opt-in would be:

1. Build your silo as usual with SiloHostBuilder and start the silo.
2. After that, you could call IClusterClient localClient = mySiloHostBuilder.GetHostedClient() or IClusterClient localClient = mySilo.GetHostedClient()

That way, we could get an IClusterClient with the hosted client implementation. Otherwise, people would just create clients as they usually do and connect to a remote silo. Less confusion, same client APIs and everyone is happy as they only use it if they really want to.

[jdom]

My concern with just a global opt in, is that it's mostly framework code that will benefit from this feature. So having a framework component opt in on behalf of the user will most likely behave as the mandatory choice, even if the end user himself doesn't want the risk of making calls outside of the grain context. So having this method is not really making it safer.
Just my 2 cents, I still prefer to have this feature than not have it, but it would be great if it's really opt in on a more granular level.

[galvesribeiro]

My concern with just a global opt in, is that it's mostly framework code that will benefit from this feature. So having a framework component opt in on behalf of the user will most likely behave as the mandatory choice

I don't see that the major use case would be framework/component developers... In fact, I never heard anyone asking for it to have an idea of embed it on their frameworks. It is quite the opposite... Everyone who I see asking for it (including myself) are looking for a way to embed SignalR/HTTP/TCP server on Orleans and make local calls to grains without have to go thru the membership/serialization/networking dance or localhost hacks that we are doing Today.

Yes, I agree with you that it must be opt-in in a sense that the user must explicitly ask for the HostedClient. However, IClusterClient APIs should be the same as in the remote one otherwise, learning curve and porting old (hacked) clients would requires too much work.

[ReubenBond]

@galvesribeiro

If that is the case, how do we get an IClusterClient with the local implementation rather then have to specify membership, address, etc?

You can get IClusterClient from the silo's container as long as you've called EnableExternalContextCalls() on the builder (it wont be added otherwise).

IMHO, a clear way to be opt-in would be:
Build your silo as usual with SiloHostBuilder and start the silo.
[...] call SiloHostBuilder.GetHostedClient()

Issues with this approach:

• The silo has started by the time you're getting the client, so the silo has already been configured and that configuration (rightfully) cannot be changed.
• Not all grain references come from IClusterClient. Some may be passed directly (eg, passed from a grain into new Thread(x), or however) or obtained via IGrainFactory, which has a common implementation (single DI container).

Yes, I agree with you that it must be opt-in in a sense that the user must explicitly ask for the HostedClient. However, IClusterClient APIs should be the same as in the remote one otherwise, learning curve and porting old (hacked) clients would requires too much work.

Note that IHostedClient is strictly an internal abstraction and the user will never need to interact with it - it's still IClusterClient/IGrainFactory for the user.

@jdom

So having this method is not really making it safer.

I pretty much agree with that.

the risk of making calls outside of the grain context.

What is the risk here?

User experience is a top priority. I am yet to see a more fine-grained opt-in/out mechanism which doesn't significantly degrade the user experience.

Users, particularly non-advanced users, should not have any concept of what is executing in an Orleans scheduler context and what is not. My problem with opt-in/opt-out is that it forces users to know about these concepts.

From my perspective, the only reason for an opt-out is if there is an issue with the implementation or design of the feature which results in unintended behavior.

[shlomiw]

@ReubenBond - once again, I'm very excited to use this feature soon!

My usages:

1. As said before, one of the classic usages, allowing the FrontEnd to easily access local grains and avoid the additional move around. Makes things easier - deployment and using the same process for the FE and Orleans. Moreover - very important for game grains if we want to reduce latency (something I'm fighting with right now..).
2. Sharing code between Orleans clients and Orleans Silos. I have common lib to handle things like subscribing stream of events which trigger other stuff, such as invoking grains, etc. I'd like to easily inject IClusterClient to this lib without thinking too much if it's a client or a silo.
   Today I do so in the silo by creating a real cluster client which has to connect the silo with all the overhead (ouch!).
3. Sometimes my grains need to use other thread contexts, such as the default thread-pool, and then I need them to be invoked again. So then the 'hacky' AsReference() with the Orleans TaskScheduler has to be involved.

How I'd address that?
For bullets 1 and 2 - after building the silo, allow getting from it IClusterClient so I could inject it to other component. So I'd expose ISiloHost.GetHostedClient().
For 3 - allow a Grain pass IClusterClient for other thread contexts in the process, then I could use the IClusterClient to invoke the Grain again. And yes - I'd need its grain ID etc.

This way you'd have the same experience when going from other contexts to Orleans and back again, only with IClusterClient.

And as I see it, if it's possible, I wouldn't add this ISiloHostBuilder.EnableExternalContextCalls(). If a developer uses the IClusterClient, then he should already know by the name of it that it's out of Orleans context.

[ReubenBond]

Enabling or disabling this feature should be done before the silo is built, IMHO.

I believe the behaviour would be unexpected if a call to GetHostedClient (or similar) at one place in the code allowed a grain reference obtained in another place in the code to start being callable.

Certainly the main way to access this feature is by accessing IClusterClient or IGrainFactory from the DI container (eg, silo.ServiceProvider.GetService<IClusterClient>()), but I didn't want to create distinctions for where a grain reference was obtained, since that opens up a lot of complicated issues and usability problems.

I still believe the best UX is for this feature to be enabled by default.

@shlomiw this PR enables all of your scenarios without extra work (other than enabling it, for now)

[SebastianStehle]

I agree to ReubenBond. When you are new to Orleans the whole backend vs frontend thing is very "scary".

[jdom]

Having this global opt in method or enabled by default is the same, so if it's not more granular, then I don't mind any further how we decide to enable it.
The points about UX are true, although I always assumed that if you are writing grain code, you should always try to be in the grain context, but you are making it sound as that is not important. Did I understand correctly? Because that's the basic reason why I'm striving to make it just a little more involved by design (just not equal, but a tiny bit harder as you saw in the suggestions) when you want to make calls outside of the context, especially when there is context that you should be using.

[ReubenBond]

@jdom I don't consider making calls from outside of a grain context to be unsafe at all. The only distinction in the code path is setting the return address of the message, but that information is not accessible from user code. When a call is made from outside of a grain context, the return address will include the hosted client's id. The return call path doesn't hit HostedClient, it's the same as for calls from grains: callback is looked up in InsideRuntimeClient based on message id and executed

[galvesribeiro]

EnableExternalContextCalls

So, when we call it, will it return a IClusterClient? If so, ok then. I just think it is a confusing method name. IMHO, it should be something that implies the user is getting the local client explicitly.

[sergeybykov]

The high-level argument for opt-in is that this feature changes the existing behavior, in certain cases significantly, and it is hard for us to predict the impact of that rather subtle change. Hence, making users that know why they need it, such as people in this discussion, add an explicit line of code seems least risky at this point, as it'll have zero unintended impact on others.

The global opt-in option allows us later to change it to a more granular way of configuring the behavior (if we figure out how to do that) and even to flip the default. I think we shouldn't change the default until at least Orleans 3.0. This will give us time to learn how the feature is used and if any adjustments are needed.

[jdom]

The reason I mentioned global opt in is almost the same as default, is that because this will be used mainly by infrastructure components (BTW @galvesribeiro I don't mean just from the Orleans team, but I mean not grain code, from whomever that comes from), then as soon a as placement directors, storage implementations, etc start using it, then the user will not have a real chance to opt out for his grain code (where the main risk is), as core components will need it.
Sure, having it as global opt in will keep us safer (in terms of the risk, not that the code is dangerous by itself) for the few weeks (month?) it will take for the first infrastructure component to use it and make it mandatory (if you want to use that component that is, hopefully it's not a core component).

[jason-bragg]

I think some of the difficulty here is that we're attempting to address a number of disparate needs that can all be addressed in some way by enabling grain calls from external threading contexts, so that capability is what's being added.

My concern with this is that context matters for a significant set of logic. Providers (storage, streams, ..) as well as facets (Transactions, future storage and streams), reminders, and other feature sets were all developed and tested with the understanding that they would be called from Orleans threading context and the single thread protections they provide. Many of these systems don't really depend on that and work fine from outside context (barring grain calls) but changing the threading expectations of existing components without testing is definitely risky. This change does not imply that these systems should now be expected to be context agnostic, as EnableExternalContextCalls only makes context irrelevant for grain calls, but it definitely blurs the lines (IMO). How do we communicate that context is only irrelevant for grain calls in a way that doesn't lead users to the assumption that this is also true for most of the other capabilities?

The current expectations are that any capability that is exposed via IClusterClient is safe to use from outside of an Orleans threading context. Many of the asks in this thread are for an IClusterClient which is available in the same process as the silo without explicitly configuring it and without the unnecessary networking overhead. If we confine our commitment to this capability (alone), the lines are no longer blurred, as the current rules remain intact, we're just making it easier and more performant as users no longer need to configure an IClusterClient themselves to talk to an in-process silo, nor incur the performance cost of the unnecessary networking.

Even if we keep the current implementation as is, and change EnableExternalContextCalls to EnableSiloClient (or something of the sort), the expected and supported patterns remain the same as is, preventing ambiguity or the expectation that users can ignore context (as it -does- matter).

Please keep in mind that having clear threading expectations are not only important for application developers but also to framework extension developers. If a team or community contributor wants to develop a new facet, stream provider, grain storage, or whatever, they need to understand the threading expectations, so having these expectations well defined is important to the extensibility, maintainability, and testability of the framework.

Side note: I'm not fond of the "hosted client" name, as multiple silos per host may be possible at some point. This client is associated with the silo, not the host.

[galvesribeiro]

Many of the asks in this thread are for an IClusterClient which is available in the same process as the silo without explicitly configuring it and without the unnecessary networking overhead. If we confine our commitment to this capability (alone), the lines are no longer blurred, as the current rules remain intact, we're just making it easier and more performant as users no longer need to configure an IClusterClient themselves to talk to an in-process silo, nor incur the performance cost of the unnecessary networking.

Yes! The only thing we want with this, is to avoid networking and serialization when calls are made from the same process, period. We still want to have the same threading guarantees that we have Today and that is why I'm saying that from an consumer perspective, I expect this to be just another implementation of IClusterClient and that it should have the exactly same functionalities (and limitations) except that you have avoided the unnecessary networking and serialization...

[jason-bragg]

To be clear, I'm entirely ok with this PR, with the soul exception of the EnableExternalContextCalls name, which in my view should indicate the enabling of a local cluster client, not a behavioral change, as long as we don't claim support of a behavioral change (at this time). The current expectations that any calls outside of an Orleans threading context should be made via an IClusterClient should remain in tact (IMO).

Should the behavioral changes introduced in this PR in relation to calls via grain references prove safe and we verify other systems function correctly without the threading guarantees provided by the Orleans threading context, then we can enable this by default and officially change what behaviors we support. After we're confident we can support the behavior change for grains and other supporting systems.

[ReubenBond]

@galvesribeiro it's not as simple as IClusterClient, but that's the main way to interact with this. I can't bring myself to implement this without also enabling the user to use IGrainFactory or direct stream provider injection or the ability to deserialize grain/stream references from storage and use them. That would be broken.

@jason-bragg what about EnableDirectClient, since it's directly attached to the silo? It's not a client specifically for the silo, though, which is why I'm less in favor of EnableSiloClient.

[attilah]

I'd put [MethodImpl(MethodImplOptions.AggressiveInlining)] on this method.

[ReubenBond]

The method probably shouldn't be inlined as-is: first we should split the throw into a different method.
Submit a PR? 😛