Code-Sharp / WampSharp

A C# implementation of WAMP (The Web Application Messaging Protocol)
http://wampsharp.net
Other
385 stars 83 forks source link

Possible to have different types for IProgress<T> and call result in reflection based callee? #353

Closed trombipeti closed 1 year ago

trombipeti commented 1 year ago

I try to explain the situation as compactly as possible:

There is an operation that is reading a larger amount of data from an external source, parses it and packs it into a nice structure (say, MyDataStructure). The data arrives slowly, but in "one piece" and not in chunks. However, the progress of the read operation can be figured out. There can be multiple operations of the same kind in progress parallel, however, they always "belong" to a separate source. I want to expose this operation through WAMP, make it cancelable and also report its progress.

What I though was possible to achieve is the following: I make a [WampProgressiveResultProcedure], have it return a Task<MyDataStructure>, and accept an IProgress<int> and a CancellationToken as two last parameters. This could then call the method for the operation, and I'd have progress reporting and cancelation automatically.

[WampProgressiveResultProcedure]
[WampProcedure("com.myapp.this_operation")]
public Task<MyDataStructure> ThisOperation(int resourceId, IProgress<int> progress, CancellationToken cancellationToken);

However, it is currently not possible with WampSharp, as apparently:

Note that the method return type should be Task<T> where this is the same T as in the IProgress<T> of the last parameter.

from https://wampsharp.net/wamp2/roles/callee/reflection-based-callee/

For me it is not obvious if it's like this because the spec says so, but I'd assume not as there are no types as such in the WAMP proto afaik.

Would it be possible to have two different Ts for progressive call results?

darkl commented 1 year ago

You are right. This is not required by the spec and also not suggested by the guidelines for TAP. I guess I made a wrong assumption when implementing this.

I am happy to support this, but unfortunately I'm extremely busy. There are other issues here that I hoped to work on, but I currently don't have the capacity to do so. If you want to perform the changes yourself then I will try to merge this. If you decide to do so, please add a unit test and try not to make too big changes.

Thanks! Elad

trombipeti commented 1 year ago

Thanks for the quick response!

I'll leave the progress reporting part out for now in our project. I won't be working for a month now, but after that I'm more than happy to make a PR - fortunately, it won't have to be too big of a change it seems, since https://github.com/Code-Sharp/WampSharp/blob/1a5bef8a63d1a3fd67aeb6f7088f4f1a139aa6e1/src/netstandard/WampSharp/WAMP2/V2/Rpc/Callee/AsyncLocalRpcOperation.cs#L86 accepts an object (it is called here: https://github.com/Code-Sharp/WampSharp/blob/1a5bef8a63d1a3fd67aeb6f7088f4f1a139aa6e1/src/netstandard/WampSharp/WAMP2/V2/Rpc/Callee/Reflection/ProgressiveAsyncMethodInfoRpcOperation.cs#L62) with value being of type T.

Making a ProgressiveAsyncMethodInfoRpcOperation<TResult, TProgress> and also adjusting the validation should be enough at first blick.

As said, I can make these changes (and tests, of course) sometime in April.

trombipeti commented 1 year ago

Just got to this issue and started fixing it. Unfortunately, I ran into some issues where I'm unsure how to do further.

The problem lies in ProgressiveAsyncOperationCallback.SetResult, which to me seems to be some abstract way to handle "results" of the calls. This is an override method from AsyncOperationCallback<TResult> and handles the progress reporting and "normal" results in one method. Unfortunately, its "result" argument has to be of type TResult, but that is then no more compatible with the IProgress<T>, if we allow different T for it (which is kind of the goal of this issue).

I kindly ask for advice on how to continue as at this point it seems to be a design decision, which I wouldn't want to (and frankly couldn't) make alone.

trombipeti commented 1 year ago

@darkl Do you think you could support me with this issue in the upcoming 1-2 weeks? If not, not a problem, but then we'll have to come up with a temporary workaround in our project. Thanks in advance!

darkl commented 1 year ago

I made some modifications so TProgeess and TResult can be different in the class you have mentioned. I hope you are able to finish the modifications needed for this feature from here.

Elad

On Wed, Apr 19, 2023, 07:29 trombipeti @.***> wrote:

@darkl https://github.com/darkl Do you think you could support me with this issue in the upcoming 1-2 weeks? If not, not a problem, but then we'll have to come up with a temporary workaround in our project. Thanks in advance!

— Reply to this email directly, view it on GitHub https://github.com/Code-Sharp/WampSharp/issues/353#issuecomment-1514650443, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIS75W67LHZB5B7BJ36ZNDXB7LC5ANCNFSM6AAAAAAVC4DGGQ . You are receiving this because you were mentioned.Message ID: @.***>

trombipeti commented 1 year ago

It seems to do the job at least partly, now I can finish the modification & add some tests.

Thank you!

trombipeti commented 1 year ago

Is there a plan on when there will be a new nuget package?

darkl commented 1 year ago

I'll try to do that this week.

Elad

darkl commented 1 year ago

Done.

trombipeti commented 1 year ago

Very nice, thank you!