Skip to content

fix(response): allow server methods to not have a response#6

Open
jayharris wants to merge 3 commits into
tjchaplin:masterfrom
jayharris:issue/noClientResponse
Open

fix(response): allow server methods to not have a response#6
jayharris wants to merge 3 commits into
tjchaplin:masterfrom
jayharris:issue/noClientResponse

Conversation

@jayharris
Copy link
Copy Markdown
Contributor

Server methods should not be required to invoke a client method in response. This change updates response messages to handle empty callData.

Fixes #5

@tjchaplin
Copy link
Copy Markdown
Owner

Thanks for the pull requests @jayharris. Looks like the CI Build is failing, could you fix the issue and re-commit?

Thanks!

* master:
  fix(polling): do not poll against closed connections
  Fix exception when using multiple hubs
@jayharris
Copy link
Copy Markdown
Contributor Author

@tjchaplin Sorry about that. I've updated the PR. Travis is now passing.

@jcoutch
Copy link
Copy Markdown

jcoutch commented Feb 19, 2017

@tjchaplin - I'm running into this issue as well. Can this get merged in and a new package deployed? Thanks!

@jcoutch
Copy link
Copy Markdown

jcoutch commented Feb 19, 2017

So, I patched the fix into my local copy of SignalRJS, and noticed that when connections are initiated for client -> server calls, while the calls succeeded, they were locked in a "pending" state on the client, which blocked all subsequent calls until the request timed out.

Tracked it down to the parseMessage function in lib/hubs/hubs.js:

	parseMessage : function(req,cb){
		this._hubDefns.forEach(function(hub){
			hub.createClientResponse(req,function(err,clientResponse,to){
				if(err	|| !clientResponse) 
					return;
				cb(clientResponse,to);
			});
		});
	},

That check for !clientResponse doesn't allow the callback to fire (which is responsible for writing the HTTP 200 back to the client.) Removing that check appears to fix this issue. This might be good to include in your patch request @jayharris...as I think the issue will become more prevalent when this patch gets merged.

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.

3 participants