{"action":"created","comment":{"url":"https://api.github.com/repos/aspnet/SignalR/pulls/comments/168360514","pull_request_review_id":96714340,"id":168360514,"diff_hunk":"@@ -22,11 +23,11 @@ internal static class SignalRClientLoggerExtensions\n         private static readonly Action<ILogger, string, string, string, string, Exception> _issueInvocation =\n             LoggerMessage.Define<string, string, string, string>(LogLevel.Trace, new EventId(4, nameof(IssueInvocation)), \"Issuing Invocation '{invocationId}': {returnType} {methodName}({args}).\");\n \n-        private static readonly Action<ILogger, string, Exception> _sendInvocation =\n-            LoggerMessage.Define<string>(LogLevel.Debug, new EventId(5, nameof(SendInvocation)), \"Sending Invocation '{invocationId}'.\");\n+        private static readonly Action<ILogger, string, HubMessage, Exception> _sendInvocation =\n+            LoggerMessage.Define<string, HubMessage>(LogLevel.Debug, new EventId(5, nameof(SendInvocation)), \"Sending Invocation '{invocationId}'. {payload}\");\n \n-        private static readonly Action<ILogger, string, Exception> _sendInvocationCompleted =\n-            LoggerMessage.Define<string>(LogLevel.Debug, new EventId(6, nameof(SendInvocationCompleted)), \"Sending Invocation '{invocationId}' completed.\");\n+        private static readonly Action<ILogger, string, HubMessage, Exception> _sendInvocationCompleted =\n+            LoggerMessage.Define<string, HubMessage>(LogLevel.Debug, new EventId(6, nameof(SendInvocationCompleted)), \"Sending Invocation '{invocationId}' completed. {payload}\");","path":"src/Microsoft.AspNetCore.SignalR.Client.Core/Internal/SignalRClientLoggerExtensions.cs","position":20,"original_position":20,"commit_id":"362d34591e9f8560342b129e87bfaff583d16ec1","original_commit_id":"d8c7397813de2c0b8f4e49a6283389931723bd90","user":{"login":"davidfowl","id":95136,"avatar_url":"https://avatars3.githubusercontent.com/u/95136?v=4","gravatar_id":"","url":"https://api.github.com/users/davidfowl","html_url":"https://github.com/davidfowl","followers_url":"https://api.github.com/users/davidfowl/followers","following_url":"https://api.github.com/users/davidfowl/following{/other_user}","gists_url":"https://api.github.com/users/davidfowl/gists{/gist_id}","starred_url":"https://api.github.com/users/davidfowl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/davidfowl/subscriptions","organizations_url":"https://api.github.com/users/davidfowl/orgs","repos_url":"https://api.github.com/users/davidfowl/repos","events_url":"https://api.github.com/users/davidfowl/events{/privacy}","received_events_url":"https://api.github.com/users/davidfowl/received_events","type":"User","site_admin":false},"body":"Yea the current logging isn’t great but that’s a good point ","created_at":"2018-02-15T01:15:17Z","updated_at":"2018-02-15T01:15:17Z","html_url":"https://github.com/aspnet/SignalR/pull/1446#discussion_r168360514","pull_request_url":"https://api.github.com/repos/aspnet/SignalR/pulls/1446","author_association":"OWNER","_links":{"self":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/comments/168360514"},"html":{"href":"https://github.com/aspnet/SignalR/pull/1446#discussion_r168360514"},"pull_request":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/1446"}},"in_reply_to_id":168302364},"pull_request":{"url":"https://api.github.com/repos/aspnet/SignalR/pulls/1446","id":169129293,"html_url":"https://github.com/aspnet/SignalR/pull/1446","diff_url":"https://github.com/aspnet/SignalR/pull/1446.diff","patch_url":"https://github.com/aspnet/SignalR/pull/1446.patch","issue_url":"https://api.github.com/repos/aspnet/SignalR/issues/1446","number":1446,"state":"open","locked":false,"title":"Change the IConnection contract to be an IDuplexPipe","user":{"login":"davidfowl","id":95136,"avatar_url":"https://avatars3.githubusercontent.com/u/95136?v=4","gravatar_id":"","url":"https://api.github.com/users/davidfowl","html_url":"https://github.com/davidfowl","followers_url":"https://api.github.com/users/davidfowl/followers","following_url":"https://api.github.com/users/davidfowl/following{/other_user}","gists_url":"https://api.github.com/users/davidfowl/gists{/gist_id}","starred_url":"https://api.github.com/users/davidfowl/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/davidfowl/subscriptions","organizations_url":"https://api.github.com/users/davidfowl/orgs","repos_url":"https://api.github.com/users/davidfowl/repos","events_url":"https://api.github.com/users/davidfowl/events{/privacy}","received_events_url":"https://api.github.com/users/davidfowl/received_events","type":"User","site_admin":false},"body":"This is a design change that we considered in the past but moved away from while we were discussiong messaging vs streams. We're firmly in the streams camp now so I think we should make this change (it also cleans and a bunch of things and introduces problems 😄 ).\r\n\r\n- The IConnection implements IDuplexPipe, it's a very simple contract, the IConnection is responsible for writing to the IDuplex pipe that is exposed. \r\n- We already changed the HttpConnection to use pipelines internal so the change there was deleting code. \r\n- This change is also required to support SignalR on top of TCP.\r\n- The server and client dispatching loop look more similar.\r\n- We were able to remove all callback handling from this layer.\r\n- The event queue is gone, the pipe is single producer and single consumer. The hub connection layer is the only one that has to implement callbacks.\r\n- With this change we don't allow running client callbacks in parallel (not sure why we even bothered before) so we were able to remove the event queue.\r\n\r\nQuirks:\r\n- Completing the pipe is messy, do we still need StopAsync and DisposeAsync? \r\n- Who is responsible for completing the PipeReader and PipeWriter exposed by the connection? At a minimum the IConnection impl needs to complete the application sides.\r\n- ~No errors if the you write to the connection before it starts or after it is disposed. This is possible but requires wrapping the PipeReader and PipeWriter returned.~ (solved)\r\n- ~The current implementation gives you a null pipe if you you don't call Start first, this can be fixed by making a wrapping PipeWriter and PipeReader.~ (solved)\r\n- IConnection.DisposeAsync doesn't own the receive loop anymore so it returns before the loop ended. This might be fixable if we hook the right events. This also makes it hard to reuse pipes.(not solved)\r\n- Today we have an ill defined contract as to which part of the client is reusable, is that part of the IConnection contract? This might make that a bit tricker.","created_at":"2018-02-14T14:47:47Z","updated_at":"2018-02-15T01:15:17Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b9704a24ed8daa2a4ba82b1f3bb341f718a7994e","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/aspnet/SignalR/pulls/1446/commits","review_comments_url":"https://api.github.com/repos/aspnet/SignalR/pulls/1446/comments","review_comment_url":"https://api.github.com/repos/aspnet/SignalR/pulls/comments{/number}","comments_url":"https://api.github.com/repos/aspnet/SignalR/issues/1446/comments","statuses_url":"https://api.github.com/repos/aspnet/SignalR/statuses/362d34591e9f8560342b129e87bfaff583d16ec1","head":{"label":"aspnet:davidfowl/iconnection-pipe","ref":"davidfowl/iconnection-pipe","sha":"362d34591e9f8560342b129e87bfaff583d16ec1","user":{"login":"aspnet","id":6476660,"avatar_url":"https://avatars3.githubusercontent.com/u/6476660?v=4","gravatar_id":"","url":"https://api.github.com/users/aspnet","html_url":"https://github.com/aspnet","followers_url":"https://api.github.com/users/aspnet/followers","following_url":"https://api.github.com/users/aspnet/following{/other_user}","gists_url":"https://api.github.com/users/aspnet/gists{/gist_id}","starred_url":"https://api.github.com/users/aspnet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aspnet/subscriptions","organizations_url":"https://api.github.com/users/aspnet/orgs","repos_url":"https://api.github.com/users/aspnet/repos","events_url":"https://api.github.com/users/aspnet/events{/privacy}","received_events_url":"https://api.github.com/users/aspnet/received_events","type":"Organization","site_admin":false},"repo":{"id":71159589,"name":"SignalR","full_name":"aspnet/SignalR","owner":{"login":"aspnet","id":6476660,"avatar_url":"https://avatars3.githubusercontent.com/u/6476660?v=4","gravatar_id":"","url":"https://api.github.com/users/aspnet","html_url":"https://github.com/aspnet","followers_url":"https://api.github.com/users/aspnet/followers","following_url":"https://api.github.com/users/aspnet/following{/other_user}","gists_url":"https://api.github.com/users/aspnet/gists{/gist_id}","starred_url":"https://api.github.com/users/aspnet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aspnet/subscriptions","organizations_url":"https://api.github.com/users/aspnet/orgs","repos_url":"https://api.github.com/users/aspnet/repos","events_url":"https://api.github.com/users/aspnet/events{/privacy}","received_events_url":"https://api.github.com/users/aspnet/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/aspnet/SignalR","description":"Incredibly simple real-time web for ASP.NET Core","fork":false,"url":"https://api.github.com/repos/aspnet/SignalR","forks_url":"https://api.github.com/repos/aspnet/SignalR/forks","keys_url":"https://api.github.com/repos/aspnet/SignalR/keys{/key_id}","collaborators_url":"https://api.github.com/repos/aspnet/SignalR/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/aspnet/SignalR/teams","hooks_url":"https://api.github.com/repos/aspnet/SignalR/hooks","issue_events_url":"https://api.github.com/repos/aspnet/SignalR/issues/events{/number}","events_url":"https://api.github.com/repos/aspnet/SignalR/events","assignees_url":"https://api.github.com/repos/aspnet/SignalR/assignees{/user}","branches_url":"https://api.github.com/repos/aspnet/SignalR/branches{/branch}","tags_url":"https://api.github.com/repos/aspnet/SignalR/tags","blobs_url":"https://api.github.com/repos/aspnet/SignalR/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/aspnet/SignalR/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/aspnet/SignalR/git/refs{/sha}","trees_url":"https://api.github.com/repos/aspnet/SignalR/git/trees{/sha}","statuses_url":"https://api.github.com/repos/aspnet/SignalR/statuses/{sha}","languages_url":"https://api.github.com/repos/aspnet/SignalR/languages","stargazers_url":"https://api.github.com/repos/aspnet/SignalR/stargazers","contributors_url":"https://api.github.com/repos/aspnet/SignalR/contributors","subscribers_url":"https://api.github.com/repos/aspnet/SignalR/subscribers","subscription_url":"https://api.github.com/repos/aspnet/SignalR/subscription","commits_url":"https://api.github.com/repos/aspnet/SignalR/commits{/sha}","git_commits_url":"https://api.github.com/repos/aspnet/SignalR/git/commits{/sha}","comments_url":"https://api.github.com/repos/aspnet/SignalR/comments{/number}","issue_comment_url":"https://api.github.com/repos/aspnet/SignalR/issues/comments{/number}","contents_url":"https://api.github.com/repos/aspnet/SignalR/contents/{+path}","compare_url":"https://api.github.com/repos/aspnet/SignalR/compare/{base}...{head}","merges_url":"https://api.github.com/repos/aspnet/SignalR/merges","archive_url":"https://api.github.com/repos/aspnet/SignalR/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/aspnet/SignalR/downloads","issues_url":"https://api.github.com/repos/aspnet/SignalR/issues{/number}","pulls_url":"https://api.github.com/repos/aspnet/SignalR/pulls{/number}","milestones_url":"https://api.github.com/repos/aspnet/SignalR/milestones{/number}","notifications_url":"https://api.github.com/repos/aspnet/SignalR/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/aspnet/SignalR/labels{/name}","releases_url":"https://api.github.com/repos/aspnet/SignalR/releases{/id}","deployments_url":"https://api.github.com/repos/aspnet/SignalR/deployments","created_at":"2016-10-17T16:39:15Z","updated_at":"2018-02-14T18:31:02Z","pushed_at":"2018-02-15T00:37:46Z","git_url":"git://github.com/aspnet/SignalR.git","ssh_url":"git@github.com:aspnet/SignalR.git","clone_url":"https://github.com/aspnet/SignalR.git","svn_url":"https://github.com/aspnet/SignalR","homepage":null,"size":4226,"stargazers_count":1436,"watchers_count":1436,"language":"C#","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":242,"mirror_url":null,"archived":false,"open_issues_count":162,"license":{"key":"other","name":"Other","spdx_id":null,"url":null},"forks":242,"open_issues":162,"watchers":1436,"default_branch":"dev"}},"base":{"label":"aspnet:dev","ref":"dev","sha":"d9b32ee3234e2f8e83377feae1ca19a380941cb3","user":{"login":"aspnet","id":6476660,"avatar_url":"https://avatars3.githubusercontent.com/u/6476660?v=4","gravatar_id":"","url":"https://api.github.com/users/aspnet","html_url":"https://github.com/aspnet","followers_url":"https://api.github.com/users/aspnet/followers","following_url":"https://api.github.com/users/aspnet/following{/other_user}","gists_url":"https://api.github.com/users/aspnet/gists{/gist_id}","starred_url":"https://api.github.com/users/aspnet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aspnet/subscriptions","organizations_url":"https://api.github.com/users/aspnet/orgs","repos_url":"https://api.github.com/users/aspnet/repos","events_url":"https://api.github.com/users/aspnet/events{/privacy}","received_events_url":"https://api.github.com/users/aspnet/received_events","type":"Organization","site_admin":false},"repo":{"id":71159589,"name":"SignalR","full_name":"aspnet/SignalR","owner":{"login":"aspnet","id":6476660,"avatar_url":"https://avatars3.githubusercontent.com/u/6476660?v=4","gravatar_id":"","url":"https://api.github.com/users/aspnet","html_url":"https://github.com/aspnet","followers_url":"https://api.github.com/users/aspnet/followers","following_url":"https://api.github.com/users/aspnet/following{/other_user}","gists_url":"https://api.github.com/users/aspnet/gists{/gist_id}","starred_url":"https://api.github.com/users/aspnet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/aspnet/subscriptions","organizations_url":"https://api.github.com/users/aspnet/orgs","repos_url":"https://api.github.com/users/aspnet/repos","events_url":"https://api.github.com/users/aspnet/events{/privacy}","received_events_url":"https://api.github.com/users/aspnet/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/aspnet/SignalR","description":"Incredibly simple real-time web for ASP.NET Core","fork":false,"url":"https://api.github.com/repos/aspnet/SignalR","forks_url":"https://api.github.com/repos/aspnet/SignalR/forks","keys_url":"https://api.github.com/repos/aspnet/SignalR/keys{/key_id}","collaborators_url":"https://api.github.com/repos/aspnet/SignalR/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/aspnet/SignalR/teams","hooks_url":"https://api.github.com/repos/aspnet/SignalR/hooks","issue_events_url":"https://api.github.com/repos/aspnet/SignalR/issues/events{/number}","events_url":"https://api.github.com/repos/aspnet/SignalR/events","assignees_url":"https://api.github.com/repos/aspnet/SignalR/assignees{/user}","branches_url":"https://api.github.com/repos/aspnet/SignalR/branches{/branch}","tags_url":"https://api.github.com/repos/aspnet/SignalR/tags","blobs_url":"https://api.github.com/repos/aspnet/SignalR/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/aspnet/SignalR/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/aspnet/SignalR/git/refs{/sha}","trees_url":"https://api.github.com/repos/aspnet/SignalR/git/trees{/sha}","statuses_url":"https://api.github.com/repos/aspnet/SignalR/statuses/{sha}","languages_url":"https://api.github.com/repos/aspnet/SignalR/languages","stargazers_url":"https://api.github.com/repos/aspnet/SignalR/stargazers","contributors_url":"https://api.github.com/repos/aspnet/SignalR/contributors","subscribers_url":"https://api.github.com/repos/aspnet/SignalR/subscribers","subscription_url":"https://api.github.com/repos/aspnet/SignalR/subscription","commits_url":"https://api.github.com/repos/aspnet/SignalR/commits{/sha}","git_commits_url":"https://api.github.com/repos/aspnet/SignalR/git/commits{/sha}","comments_url":"https://api.github.com/repos/aspnet/SignalR/comments{/number}","issue_comment_url":"https://api.github.com/repos/aspnet/SignalR/issues/comments{/number}","contents_url":"https://api.github.com/repos/aspnet/SignalR/contents/{+path}","compare_url":"https://api.github.com/repos/aspnet/SignalR/compare/{base}...{head}","merges_url":"https://api.github.com/repos/aspnet/SignalR/merges","archive_url":"https://api.github.com/repos/aspnet/SignalR/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/aspnet/SignalR/downloads","issues_url":"https://api.github.com/repos/aspnet/SignalR/issues{/number}","pulls_url":"https://api.github.com/repos/aspnet/SignalR/pulls{/number}","milestones_url":"https://api.github.com/repos/aspnet/SignalR/milestones{/number}","notifications_url":"https://api.github.com/repos/aspnet/SignalR/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/aspnet/SignalR/labels{/name}","releases_url":"https://api.github.com/repos/aspnet/SignalR/releases{/id}","deployments_url":"https://api.github.com/repos/aspnet/SignalR/deployments","created_at":"2016-10-17T16:39:15Z","updated_at":"2018-02-14T18:31:02Z","pushed_at":"2018-02-15T00:37:46Z","git_url":"git://github.com/aspnet/SignalR.git","ssh_url":"git@github.com:aspnet/SignalR.git","clone_url":"https://github.com/aspnet/SignalR.git","svn_url":"https://github.com/aspnet/SignalR","homepage":null,"size":4226,"stargazers_count":1436,"watchers_count":1436,"language":"C#","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":242,"mirror_url":null,"archived":false,"open_issues_count":162,"license":{"key":"other","name":"Other","spdx_id":null,"url":null},"forks":242,"open_issues":162,"watchers":1436,"default_branch":"dev"}},"_links":{"self":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/1446"},"html":{"href":"https://github.com/aspnet/SignalR/pull/1446"},"issue":{"href":"https://api.github.com/repos/aspnet/SignalR/issues/1446"},"comments":{"href":"https://api.github.com/repos/aspnet/SignalR/issues/1446/comments"},"review_comments":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/1446/comments"},"review_comment":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/aspnet/SignalR/pulls/1446/commits"},"statuses":{"href":"https://api.github.com/repos/aspnet/SignalR/statuses/362d34591e9f8560342b129e87bfaff583d16ec1"}},"author_association":"OWNER"}}
{"actor":{"display_login":"davidfowl"}}