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

Client <-> Server return values and exceptions. #65

Merged
merged 16 commits into from
Apr 26, 2018

Conversation

Henry00IS
Copy link
Collaborator

@Henry00IS Henry00IS commented Apr 20, 2018

Here is an example. Assume we have this method on the server:

image
(sorry, the first argument is only in my library, bad screenshot on my part)


We call it on the client with parameters 7 and 3 to get result 10:

image


When we call it with parameters 0 and 3 we get the exception as expected:

image

Here is a flowchart of how I associate invocations and return values:

untitled diagram

There is a cancellation token in place so that if it has been waiting for a reply from the server for 60 seconds it will give up automatically and throws an exception. Please carefully review the code and feel free to come up with suggestions. This is likely going to be a breaking change. Server to Client is coming up soon...

@Henry00IS
Copy link
Collaborator Author

Henry00IS commented Apr 20, 2018

I have added Server to Client return values and exceptions as well. It appears I broke some of the example projects in the process according to Travis CI because I changed the .On() method to use Func with a return value. (Edit: I have not tested the .On() methods as I have my own system, should work though)

@Henry00IS
Copy link
Collaborator Author

I fixed that all the JSON types got obliterated. No more longs and JArrays.

@danielohirsch
Copy link

Hi! Any chance I can get your changes? I'd love to try to work on this pull...

@radu-matei
Copy link
Owner

Hi, @danielohirsch!
You can follow this guide in order to checkout a pull request - https://help.github.com/articles/checking-out-pull-requests-locally/

@Henry00IS
Copy link
Collaborator Author

Hi, @danielohirsch!
If you are as amazingly confused as me that these guys can checkout and magically contribute to pull requests, you can also just get it directly from the branch on my fork to try it out.

@danielohirsch
Copy link

Thanks Henry - I just ended up copying in all of your new and changed files...
But I am having an issue with the web example... the values returned are undefined... not sure why - I traced it through the backend and it was putting the value through all the way - but when it gets to the js that handles it - when I trace through the js in chrome - the returned value is undefined... Were you able to get the web examples working?

@Henry00IS
Copy link
Collaborator Author

Ah, I haven't fixed the example projects yet. This will work with C# to C# communication. We may have to make a custom JavaScript include that can handle the more specific changes I proposed in this PR. You can't use return values there (which are optional anyway) but it's especially missing the $type variables in JSON that helps C# to keep track of bool/int/string[]/long/byte etc. so it's a lot more reflection friendly.

@danielohirsch
Copy link

Gotcha - I am digging through your changes... nice work!
I am curious - is it possible to do a return value the other direction? (in the C# to C# world) - where the server sends a message to a client handler and the client returns a payload/value?

@Henry00IS
Copy link
Collaborator Author

Henry00IS commented Apr 20, 2018

Yeah that works already! The .On now takes a Func. I haven't tested the .On method as I am using reflection identical to the server but you should be able to write something like:

connection.On("Test", (args) => {
    return 5 + 5;
});

I mean... I am breaking so much in this PR. I might as well publish my client-side non-.On() solution that is so much better.

@danielohirsch
Copy link

I'd love to take a look at it.

@Henry00IS
Copy link
Collaborator Author

Henry00IS commented Apr 20, 2018

On the server inherit from WebSocketHandler and override the:

        public override async Task<object> OnInvokeMethodReceived(WebSocket socket, InvocationDescriptor invocationDescriptor)
        {
            // there must be a separator in the method name.
            if (!invocationDescriptor.MethodName.Contains('/')) throw new Exception("Invalid method name.");

            // find the controller and the method name.
            string[] names = invocationDescriptor.MethodName.Split('/');
            string controller = names[0].ToLower();
            string command = "" + names[1];

            // use reflection to find the method in the desired controller.
            object self = null;
            MethodInfo method = null;
            switch (controller)
            {
                case "session": // add more to this list.
                    self = m_SessionController; // make some class containing your methods.
                    method = typeof(SessionController).GetMethod(command);
                    break;

                default:
                    throw new Exception($"Received command '{command}' for unknown controller '{controller}'.");
            }

            // if the method could not be found:
            if (method == null)
                throw new Exception($"Received unknown command '{command}' for controller '{controller}'.");

            // insert client as parameter. this is totally worth it but this won't compile for you.
            // List<object> args = invocationDescriptor.Arguments.ToList();
            // args.Insert(0, m_Clients[WebSocketConnectionManager.GetId(socket)]);

            // call the method asynchronously.
            return await Task.Run(() => method.Invoke(self, args.ToArray()));
        }

This will allow you to call methods in different classes "controllers":

        public int ᐅDoMath(/*WebSocketClient client,*/ int a, int b)
        {
            if (a == 0 || b == 0) throw new Exception("I am disappointed in you son.");

            return a + b;
        }

Using a simple prefix in the name:

        int value = await connection.SendAsync<int, int, int>("session/DoMath", 5, 5);

On the client inherit from Connection and override the:

        protected override async Task<object> Invoke(InvocationDescriptor invocationDescriptor)
        {
            // there must be a separator in the method name.
            if (!invocationDescriptor.MethodName.Contains('/')) return null;

            // find the controller and the method name.
            string[] names = invocationDescriptor.MethodName.Split('/');
            string controller = names[0].ToLower();
            string command = "" + names[1];

            // use reflection to find the method in the desired controller.
            object self = null;
            MethodInfo method = null;
            switch (controller)
            {
                case "session": // add more to this list.
                    self = m_SessionController; // make some class containing your methods.
                    method = typeof(SessionController).GetMethod(command);
                    break;

                default:
                    //App.MainWindow.ViewModel.StatusBarMessage = $"Received command '{command}' for unknown controller '{controller}'.";
                    return null;
            }

            // if the method could not be found:
            if (method == null)
            {
                //App.MainWindow.ViewModel.StatusBarMessage = $"Received unknown command '{command}' for controller '{controller}'.";
                return null;
            }

            // call the method asynchronously.
            return await Task.Run(() => method.Invoke(self, invocationDescriptor.Arguments.ToArray()));
        }

This will allow you to call methods in different classes "controllers":

        public void ᐅOnSessionIdentifiers(string[] identifiers)
        {
            System.Windows.MessageBox.Show("Received " + identifiers.Count() + " identifiers!");
        }

Using a simple prefix in the name:

        await WebSocketManager.InvokeClientMethodOnlyAsync(client.Identifier, "session/OnSessionIdentifiers", GetSessionIdentifiers().ToArray());

(edit: you can return values too. just my example uses void here.)


I use the beautiful ᐅ character to prevent people calling methods they aren't supposed to as well as indicating that this method is called through reflection (as it has 0 references, don't want other programmers deleting them thinking it's unused code and git).

@Henry00IS
Copy link
Collaborator Author

I mean if everyone is okay with it I would do what @RobSchoenaker proposed here. And just remove .On() completely because it's a mess and no-one wants to bother with an object[] for the parameters. And replace it with some proper reflection logic just like the server and my post above.

@Henry00IS
Copy link
Collaborator Author

Also is anyone able to figure out why these lines:

int value = await connection.SendAsync<int, int, int>("session/DoMath", 5, 5);

Don't have type inference? Is it because it returns a Task that breaks it? It's sad that you have to specify all of the types now...

@danielohirsch
Copy link

It works for me... with a string
var taskResult = _connection.SendAsync<string, string>("SayHello", "Test Client");
taskResult.Await();
var result = taskResult.Result;

in a static method - so calling Wait on the task instead of await

…tionStrategy, ControllerMethodInvocationStrategy, DecoratedControllerMethodInvocationStrategy.
@Henry00IS
Copy link
Collaborator Author

Henry00IS commented Apr 23, 2018

I have added several method invocation strategies for the client (work in progress). The prefix is totally optional but I like ᐅ hehe. ;)

StringMethodInvocationStrategy

var strategy = new StringMethodInvocationStrategy();
var connection = new Connection(strategy);
strategy.On("Session/OnSessionIdentifiers", (args) =>
{
	System.Windows.MessageBox.Show("Received " + args.Length + " arguments!");
});

ControllerMethodInvocationStrategy

var strategy = new ControllerMethodInvocationStrategy("", new TestController());
var connection = new Connection(strategy);

public class TestController
{
	// called as 'OnSessionIdentifiers'.
	public void ᐅOnSessionIdentifiers(WebSocket socket, string[] identifiers)
	{
		System.Windows.MessageBox.Show("Received " + identifiers.Count() + " identifiers!");
	}
}

DecoratedControllerMethodInvocationStrategy

var strategy = new DecoratedControllerMethodInvocationStrategy("");
var connection = new Connection(strategy);
strategy.Register("Session", new TestController());
// can also set a custom separator instead of '/'.
// called as 'Session/OnSessionIdentifiers'.

@Henry00IS Henry00IS changed the title Client to Server return values and exceptions. Client <-> Server return values and exceptions. Apr 23, 2018
@Henry00IS
Copy link
Collaborator Author

When you throw exceptions, because it's reflection that invokes methods, they will show up as user unhandled but they are handled. You can fix this behavior as described here.

Copy link
Collaborator

@RobSchoenaker RobSchoenaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some room for improvement. Looks good!

@RobSchoenaker
Copy link
Collaborator

@Henry00IS can you please double check the failing build?

@Henry00IS
Copy link
Collaborator Author

Awesome! And yeah it's the example project(s) that have to be modified.

…o it's not converted to a string (which causes an exception).
@Henry00IS
Copy link
Collaborator Author

Good news, found a legitimate excuse to fix up JavaScript for this new system (as I only work on this due to work). So will be making a small JavaScript include library that takes care of some overhead and by doing so fix the example projects.

WebSocketConnectionManager: Now preventing several exceptions on the disconnecting of a client.
PrimitiveJsonConverter: Whitelisted System.String as a primitive type so Json.Net doesn't generate a simple array of strings when the JS Client expects $type+$value entries.
@Henry00IS
Copy link
Collaborator Author

I think I covered all of it. The chat example uses all of the new features in one way or another.

…ll all waiting callbacks with an error so the program execution can continue properly.
@Henry00IS
Copy link
Collaborator Author

At this point we can merge it and hope that people will contribute to it to make it even better.

@RobSchoenaker RobSchoenaker merged commit 7d27e5b into radu-matei:master Apr 26, 2018
@Henry00IS Henry00IS deleted the C2S branch April 26, 2018 12:48
@RobSchoenaker
Copy link
Collaborator

As requested. Merged. I still need to merge my original changes too - too busy @ work 🙂

@Henry00IS
Copy link
Collaborator Author

This is now officially my favorite networking library. 😁
Can't wait to use this outside of work as well! ~
I think we just changed the world. Let's wait and see what happens.

@danielohirsch
Copy link

Nice job Henry

@radu-matei
Copy link
Owner

Suuuper excited about this merge!
Congrats to @Henry00IS and @RobSchoenaker for the awesome work!

@helmishariff
Copy link

is there any sample in example project?

@helmishariff
Copy link

ok. I found it.

@Henry00IS
Copy link
Collaborator Author

For anyone reading this in the future, have a look at the ChatApplication sample.

@helmishariff
Copy link

from javascript I can use connection.methods. to return value but how to return value in c# client?

@Henry00IS
Copy link
Collaborator Author

When you create a new C# Connection, you pass a MethodInvocationStrategy in the constructor. It's identical to how you'd make a server method (depending on the invocation strategy you decide to use).

public int DoMath(int a, int b)
{
    if (a == 0 || b == 0) throw new Exception("That makes no sense.");
    return a + b;
}

@helmishariff
Copy link

I'm sorry. It's something like this?

_strategy.On("CallMethod", (arguments) =>
{
return "test";
});

From server it will return value "test" when call InvokeClientMethodAsync.

@Henry00IS
Copy link
Collaborator Author

Henry00IS commented May 29, 2018

Sure, that's the StringMethodInvocationStrategy, if you prefer that then go for it. :)
Just very annoying with the "arguments". If you wish to write methods like I did use the ControllerMethodInvocationStrategy.

Just make a class with a method like I posted above and pass it to the new ControllerMethodInvocationStrategy(myClass). It will use reflection and automagically call the methods that it finds in myClass.

int result = await InvokeClientMethodAsync<int, int, int>(clientid, "DoMath", 3, 5);

@helmishariff
Copy link

I tried using StringMethodInvocationStrategy but can't get return value.
_
strategy.On("myMethod", (arguments) =>
{
return "test";
});
string ret = await InvokeClientMethodAsync(wsId, "myMethod", obj);

But I can't get the ret value. I assume I should get value "test".

@helmishariff
Copy link

I tried using ControllerMethodInvocationStrategy but I get error.

ControllerMethodInvocationStrategy _ctrlStrategy = new ControllerMethodInvocationStrategy(InnerHandler);

public class InnerHandler
{
public string receiveMessages(string id, string msg)
{
return "success";
}
}

@helmishariff
Copy link

I can call method using ControllerMethodInvocationStrategy but can't get return value.
I get timeout:
System.Threading.Tasks.TaskCanceledException: A task was canceled.

@helmishariff
Copy link

I tried but it freeze. Than I get timeout exception. No problem if I invoke the method on the client without return value.
int result = await InvokeClientMethodAsync<int, int, int>(clientid, "DoMath", 3, 5);

From client sent to server I had no problem to get return value.

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

Successfully merging this pull request may close these issues.

5 participants