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

Non-serialized hub invocations #5351

Closed
Tragetaschen opened this issue Oct 15, 2018 · 18 comments · Fixed by #23535
Closed

Non-serialized hub invocations #5351

Tragetaschen opened this issue Oct 15, 2018 · 18 comments · Fixed by #23535
Assignees
Labels
area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@Tragetaschen
Copy link
Contributor

I have tried to circumvent the missing stream invocations in the Java client by introducing for each streaming method on my Hub a sibling method that would await and return the next value that a ChannelReader would provide. The client would then have to immediately invoke the same method again after receiving a value to simulate continuous values. In my C# design validation client, I try to invoke two methods, run Task.WhenAny and then re-issue the finished invocation.

However, the invocation serialization from 1957655 means that any additional incoming message on the connection will only be dispatched when a running (async-blocking) invocation has finished. This includes any second invocation, a new streaming invocation and (which is where it's likely getting interesting) any form of graceful connection abort from the client. As far as I could see in the code for the latter case, all forms of Abort (which would trigger the cancellation token) originate from outside the dispatch loop, so only actually reading from the input pipe can close a connection which is blocked by the running invocation.

So: Personally, I'd love to see lifting the restriction around non-streaming invocations...

@analogrelay
Copy link
Contributor

We can revisit this in 3.0. We intentionally disabled concurrent invocations because there was some concern around memory issues. We lifted the restriction for streaming invocations because they don't make sense with that restriction. We can revisit this though in 3.0

@aspnet-hello aspnet-hello transferred this issue from aspnet/SignalR Dec 17, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0 milestone Dec 17, 2018
@aspnet-hello aspnet-hello added area-signalr Includes: SignalR clients and servers cost: M labels Dec 17, 2018
@BrennanConroy
Copy link
Member

For design: Here is a short conversation about making the invocations serial aspnet/SignalR#2086 (comment)

@bradygaster bradygaster modified the milestone: 3.0.0 Feb 6, 2019
@analogrelay analogrelay added release-3.0 Needs: Design This issue requires design work before implementating. and removed status: Needs Design labels Mar 21, 2019
@analogrelay analogrelay modified the milestones: 3.0.0, 3.0.0-preview4 Mar 25, 2019
@analogrelay
Copy link
Contributor

Let's talk about a design for this in preview 4 (implementation may wait). We probably need this to be opt-in behavior to reduce the risk of user async code going haywire.

@analogrelay
Copy link
Contributor

Sounds like this may be important for Razor Components (cc @SteveSandersonMS @davidfowl @rynowak @javiercn)

@SteveSandersonMS
Copy link
Member

@anurse It's not required for Razor Components, because Razor Components has its own sync context anyway. As far as SignalR is concerned, all the calls into ComponentHub are fire-and-forget (or will be, once we fix one outstanding bug).

@davidfowl
Copy link
Member

We should do this regardless.

@analogrelay
Copy link
Contributor

@davidfowl I don't disagree, it just doesn't align with any of our epics so the prioritization is a little different. If Razor Components had required it, that would have changed the prioritization.

@davidfowl
Copy link
Member

@anurse Where is the "we have bugs" epic?

@ddweber
Copy link

ddweber commented Sep 20, 2019

Are there any updates on this? I think in regard to long running server methods this can be considered a blocker like stated in #10257 (Client timeout..). The suggested workarounds are not really helpfull neither. And it has been moved around for quite some time.

Waiting on this issue to be resolved my workaround was to increase the hubOptions.ClientTimeoutInterval temporarly, but this leads to other problems..

@analogrelay
Copy link
Contributor

It's in the 5.0 milestone. We'll look at it then.

@halter73
Copy link
Member

The suggested workarounds are not really helpfull neither.

While I agree it would be better to allow non-serialized hub invocations for all invocation types, streaming invocations do allow for parallel hub method calls from a single client.

Why is this workaround not helpful @ddweber?

@ddweber
Copy link

ddweber commented Sep 23, 2019

Quote from @anurse in #10257:

Another super hacky way you could do this is with the streaming support, since we don't block the message loop on those requests. You can just stream only a single item back :). It's a big hack though and a breaking change to your Client if you switch back and forth between streaming and non-streaming.

For development that is ok but once deployed in production that is really a breaking change..
All client implementations will need to be refactored/updated.

@ddweber
Copy link

ddweber commented Oct 29, 2019

@halter73 Another problem i am facing is that the java client does not support streaming by now (according to this documentation)..

@halter73
Copy link
Member

@ddweber The doc link you posted is for version 2.2 of the Java client. In 3.0, the Java client does support server-to-client streaming which is all that's necessary for non-serialized hub invocations.

@analogrelay
Copy link
Contributor

I'd also suggest looking at our top-level client features matrix: https://docs.microsoft.com/en-us/aspnet/core/signalr/client-features?view=aspnetcore-3.0 it's a good way to view the supported features and which versions support them.

@ddweber
Copy link

ddweber commented Oct 30, 2019

Oh my fault, sorry. I did not notice that my bookmark pointed to a fixed older version :(
Thank you both! :) I´ll give it a try.

@BrennanConroy BrennanConroy self-assigned this Jun 1, 2020
@BrennanConroy BrennanConroy modified the milestones: 5.0.0, 5.0.0-preview7 Jun 8, 2020
@BrennanConroy BrennanConroy removed the Needs: Design This issue requires design work before implementating. label Aug 19, 2020
@analogrelay
Copy link
Contributor

Wesley Crusher from Star Trek: The Next Generation steps back from the camera and says "Wow!"

@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants