Skip to content

Throw from client IInvocationBinder.GetParameterTypes when the target method does not exist - #67939

Open
Arul1998 wants to merge 3 commits into
dotnet:mainfrom
Arul1998:fix/invocation-binder-missing-method
Open

Throw from client IInvocationBinder.GetParameterTypes when the target method does not exist#67939
Arul1998 wants to merge 3 commits into
dotnet:mainfrom
Arul1998:fix/invocation-binder-missing-method

Conversation

@Arul1998

Copy link
Copy Markdown
  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Throw from client IInvocationBinder.GetParameterTypes for unknown methods

Description

On the C# client, IInvocationBinder.GetParameterTypes (in HubConnection.ConnectionState) returned Type.EmptyTypes when no handler was registered for the target method. Hub protocols then bound the incoming arguments against an empty parameter list, producing a misleading result:

  • Invocation with arguments → System.IO.InvalidDataException: Invocation provides N argument(s) but target expects 0.
  • Invocation with no arguments → no error surfaced at all.

In both cases the real problem — the method does not exist — was hidden.

This change makes the client throw HubException($"Method '{methodName}' does not exist."), matching the server's DefaultHubDispatcher.GetParameterTypes, which already throws for the same scenario. The existing MissingHandler warning log is retained, and the IInvocationBinder interface docs now state that implementations should throw when the invocation/method cannot be found (per @BrennanConroy's guidance in the issue).

The connection is not torn down: protocols wrap the GetParameterTypes call and surface the failure as an InvocationBindingFailureMessage, so the exception becomes a logged ArgumentBindingFailure with a clear "method does not exist" message and processing continues.

Behavior change to note for review: for a result-expecting invocation targeting an unregistered method, the completion error sent back to the server changes from "Client didn't provide a result." to "Client failed to parse argument(s).", since binding now fails first. The ClientResultReturnsErrorIfNoHandlerFromClient test is updated accordingly.

Fixes #63084

… method does not exist

Matches DefaultHubDispatcher on the server. Previously the client binder returned Type.EmptyTypes for unknown methods, so hub protocols reported a misleading 'Invocation provides N argument(s) but target expects 0' binding error - or, for zero-argument invocations, silently fell through to the missing-handler path.

Fixes dotnet#63084
@BrennanConroy

Copy link
Copy Markdown
Member

Test InvokeNonExistantClientMethodFromServer is failing with this change.

@Arul1998

Copy link
Copy Markdown
Author

Good catch, thanks fixed. The invocation now fails to bind before dispatch, so the client logs ArgumentBindingFailure (Error) where it previously logged only MissingHandler (Warning); I've updated the test to expect that.

That raises a question worth your call: escalating this path to Error means a routine Clients.All.SendAsync("X") will log an Error on every client that doesn't handle X. If you'd rather not change that, I can keep the binder throwing (so custom IHubProtocol implementations get the clear "method does not exist" signal this issue is about) while having HubConnection recognize that specific failure and continue logging it as the existing MissingHandler warning which would leave this test unchanged. Happy to do that instead if you prefer.

@BrennanConroy

Copy link
Copy Markdown
Member

That's a good point. Error is more for issues that likely indicate application bugs or something really went wrong. Ignoring certain method invocations from the server can be intentional. If it's easy, we should try to avoid an error log. I'm worried it won't be particularly easy here though since it's multiple layers between the exception and where we log.

Following review feedback, an intentional server invocation of a method
the client doesn't handle (e.g. a broadcast) should not be logged as an
error. The binder now throws a dedicated HubMethodDoesNotExistException,
which the message loop recognizes to skip the ArgumentBindingFailure
error log while still surfacing genuine argument-binding failures as
errors. The client-result wire response is unchanged from before this PR.
@Arul1998

Copy link
Copy Markdown
Author

Turned out to be straightforward. The binder now throws a dedicated internal HubMethodDoesNotExistException, and the one place we handle InvocationBindingFailureMessage checks for it and skips the error log a genuine argument-binding failure on a registered handler still logs ArgumentBindingFailure at Error. The client-result response is back to the pre-existing "Client didn't provide a result.", and the tests now assert a warning with no error.

One small note: a client-result invocation of an unhandled method now logs MissingHandler rather than MissingResultHandler (the binder only sees the method name, so it can't distinguish the two), but both are warnings and the wire response is unchanged. Happy to preserve MissingResultHandler if you'd prefer.

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.

IInvocationBinder throws or returns empty array inconsistently

2 participants