Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timing out reflection-based callers #267

Open
StevenBonePgh opened this issue Aug 22, 2018 · 10 comments
Open

Timing out reflection-based callers #267

StevenBonePgh opened this issue Aug 22, 2018 · 10 comments

Comments

@StevenBonePgh
Copy link
Contributor

I had a scenario where there was a transport error attempting to send the return value of an RPC to the caller. The error was short lived/transient, and it did not appear to cause the WebSocket to close at lease on the client side. The caller hung awaiting a response. Sadly, I lost the log file for both router and caller and this scenario is not exactly reproducible on demand. I recall seeing a goodbye right after the error was logged and am unsure of exactly what the state of anything actually was, including the state of the websocket and/or session.

As I was attempting to deterministically reproduce this scenario it struck me to create a JsonConverter that throws an exception during serialization of the type being returned by the reflection based callee. This did not end up throwing an exception to the client via an error response as I kind of expected. Obviously, this is something that should rarely/never happen so I don't know if I'd consider this to be a bug. I was hoping to mimic an issue with transport and potentially reproduce a similar state as above.

As I was thinking about how to resolve this class of problems (needing to have a timeout on the caller side) in my own application, I considered generating a proxy over the reflection base caller's interface/proxy to ease the burden of repetitive code to handle timeout on the response. Initially, I considered using Castle.Core.AsyncInterceptor.

It seems to me that this kind of feature is ripe for inclusion in the base WampSharp distribution. Rather than an interceptor, one could build off the Raw implementations and provide default strategies for handling timeout on all RPC calls, perhaps overridable on a per Uri basis using attributes for reflection bases use. Are you open to entertaining some ideas in this regard, or have you considered a recipe-based approach for handling this?

@darkl
Copy link
Member

darkl commented Aug 22, 2018

I don't plan implementing a built-in timeout feature in the near future. For now, the correct usage should be via the CancellationToken feature support. Something like this example

public interface IAdd2ServiceProxy
{
    [WampProcedure("com.example.add2")]
    Task<int> Add2Async(int a, int b, CancellationToken token);
}

IAdd2ServiceProxy proxy =
    channel.RealmProxy.Services
           .GetCalleeProxy<IAdd2ServiceProxy>();

CancellationTokenSource cancellationTokenSource = new CancellationTokenSource();

Task<int> task = proxy.Add2Async(3, 4, cancellationTokenSource.Token);

await Task.WhenAny(task, Task.Delay(5000));

if (!task.IsCompleted)
{
    cancellationTokenSource.Cancel();
}

Note that this does not complete the task immediately - it sends a request to the router to cancel the CALL request, and only when it receives an ERROR message from the router, it sets the rpc task's exception to the received error.

If you have an idea how you would like this to be built in WampSharp, please write here. Perhaps we can add a method to the CallerInterceptor which specifies a timeout for a given procedure. However, it might take me a while to implement this.

@StevenBonePgh
Copy link
Contributor Author

Perhaps CallOptions could be used (the internal property TimeoutMili). Then in ClientInvocationHandler the CallOptions can be provided to WaitForResult and AwaitForResult, the latter of which could start like so:

Task<T> operationTask = asyncOperationCallback.Task;

Task<Exception> disconnectionTask = mDisconnectionTaskCompletionSource.Task;

TimeSpan waitSpan;
if (callOptions.TimeoutMili.HasValue)
    waitSpan = TimeSpan.FromMilliseconds(callOptions.TimeoutMili.Value);
else
    waitSpan = Timeout.InfiniteTimeSpan;

Task timeoutTask = Task.Delay(waitSpan);

Task task = await Task.WhenAny(operationTask,
                               disconnectionTask,
                               timeoutTask)
                      .ConfigureAwait(false);

registration.Dispose();

if (timeoutTask.IsCompleted)
{
    var exception = new WampException("Timeout");
    asyncOperationCallback.SetException(exception);
//...

I'm not 100% on the most correct implementation of WaitForResult and AwaitForResult given the several potential scenarios, specifically the IObservable for non async compilation. I'm not sure we would need to worry about cancelling the call (or at least elide awaiting the cancellation response) in the timeout case - perhaps this behavior is a different call option. Also, should have a specific exception type for a timeout.

Using CallOptions for this purpose at first glance felt a bit 'unpure' as these map to WAMP protocol things - perhaps the property name could be prefixed by 'Ws' to indicate this property is WampSharp specific.

As each method has an interceptor with a copy of CallOptions, we could add an attribute that would allow a per-method override of the call option timeout via a bit of extra implementation in CalleeProxyInterceptor.GetCallOptions.

Let me know if I can help - as described above I don't think it is a terrible amount of code, but my comfort level is pretty low inside ClientInvocationHandler - especially if cancellation is desired as part of this.

@darkl
Copy link
Member

darkl commented Aug 23, 2018

The WAMP spec actually discusses timeouts in the CallOptions argument. Unfortunately, it is not implemented by the popular implementations. See these issues #870, #299. So maybe that option is not so unpure.

Also the current spec gives an amount of freedom on how to implement call timeouts. This actually complicates things, as a CANCEL message might be redundant for some router implementations and necessary for others.

Elad

@hawkerm
Copy link

hawkerm commented Mar 10, 2019

Is this related to the ClientInvocationHandler WaitForResult timeout I'm seeing? I'm making a call to a void Disconnect(int id) method on my router proxy.

However, it's getting stuck here in the callee reflection code waiting on an infinite timeout.

I see the method complete on my router, so I'm confused on why it's still waiting for a result?

@darkl
Copy link
Member

darkl commented Mar 10, 2019

@hawkerm if you see calls to WaitForResult this probably means you are using synchronous rpc calls. It is easy to make an error and accidentally block the WebSocket thread using synchronous rpc calls, which is probably what you are experiencing. In order to avoid this, it is better to use the asynchronous api syntax.

Elad

@hawkerm
Copy link

hawkerm commented Mar 10, 2019

@darkl thanks, I'll try that, assuming it's just making the interface have the async/task signature like called out here?

Is there a way to find out what else may be blocking the WebSocket thread if I did want to make it easier to use synchronous calls?

@darkl
Copy link
Member

darkl commented Mar 10, 2019

You only need to change the signatures of your CalleeProxy interface to return Tasks in order to use the async api, see also Reflection Based Caller.

If you want to find the blocking code, you should look what happens before your code calls the rpc. It is likely is that you awaited the channel.Open() call, which made you continue on the WebSocket thread, and then you blocked using a synchronous rpc call.

Elad

@hawkerm
Copy link

hawkerm commented Mar 11, 2019

@darkl I switched over to async code now, as I am awaiting the channel Open.

I'm seeing an intermittent error in the async waiting in ClientInvocationHandler here now sometimes. With the message WampSharp.V2.Core.Contracts.WampException: wamp.error.runtime_error.

It seems to be happening when I'm running multiple test cases in a row, but works fine when I run my test case alone. Which seems a little odd.

@darkl
Copy link
Member

darkl commented Mar 11, 2019

Runtime error means that your callee threw an exception. You can look at the details to see what exception was thrown.

Elad

@hawkerm
Copy link

hawkerm commented Mar 12, 2019

@darkl thanks that was a great help!

I had a test case that was getting mutated from another case somehow. So the asset was failing in an event callback I had that was apparently on the WebSocket thread. I need to look into isolating that event callback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants