{"action":"created","comment":{"url":"https://api.github.com/repos/netty/netty/pulls/comments/83029617","pull_request_review_id":3896427,"id":83029617,"diff_hunk":"@@ -168,72 +216,54 @@ protected final void doWrite(ChannelOutboundBuffer in) throws Exception {\n         if (closed) {\n             throw CLOSED_CHANNEL_EXCEPTION;\n         }\n-\n-        EventExecutor preferredExecutor = preferredEventExecutor();\n-\n-        // TODO: this is pretty broken; futures should only be completed after they are processed on\n-        // the parent channel. However, it isn't currently possible due to ChannelOutboundBuffer's\n-        // behavior which requires completing the current future before getting the next message. It\n-        // should become easier once we have outbound flow control support.\n-        // https://github.com/netty/netty/issues/4941\n-        if (preferredExecutor.inEventLoop()) {\n-            for (;;) {\n-                Object msg = in.current();\n-                if (msg == null) {\n-                    break;\n-                }\n-                try {\n-                    doWrite(ReferenceCountUtil.retain(msg));\n-                } catch (Throwable t) {\n-                    // It would be nice to fail the future, but we can't do that if not on the event\n-                    // loop. So we instead opt for a solution that is consistent.\n-                    pipeline().fireExceptionCaught(t);\n-                }\n-                in.remove();\n+        final MessageSizeEstimator.Handle sizeEstimator = config().getMessageSizeEstimator().newHandle();\n+        for (;;) {\n+            final Object msg = in.current();\n+            if (msg == null) {\n+                break;\n             }\n-            doWriteComplete();\n-        } else {\n-            // Use a copy because the original msgs will be recycled by AbstractChannel.\n-            final Object[] msgsCopy = new Object[in.size()];\n-            for (int i = 0; i < msgsCopy.length; i ++) {\n-                msgsCopy[i] = ReferenceCountUtil.retain(in.current());\n+            // TODO(buchgr): Detecting cancellation relies on ChannelOutboundBuffer internals. NOT COOL!","path":"codec-http2/src/main/java/io/netty/handler/codec/http2/AbstractHttp2StreamChannel.java","position":159,"original_position":159,"commit_id":"cab93db7d7232cdc2602491454e374324ca86c64","original_commit_id":"cab93db7d7232cdc2602491454e374324ca86c64","user":{"login":"netkins","id":9507557,"avatar_url":"https://avatars.githubusercontent.com/u/9507557?v=3","gravatar_id":"","url":"https://api.github.com/users/netkins","html_url":"https://github.com/netkins","followers_url":"https://api.github.com/users/netkins/followers","following_url":"https://api.github.com/users/netkins/following{/other_user}","gists_url":"https://api.github.com/users/netkins/gists{/gist_id}","starred_url":"https://api.github.com/users/netkins/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/netkins/subscriptions","organizations_url":"https://api.github.com/users/netkins/orgs","repos_url":"https://api.github.com/users/netkins/repos","events_url":"https://api.github.com/users/netkins/events{/privacy}","received_events_url":"https://api.github.com/users/netkins/received_events","type":"User","site_admin":false},"body":"![INFO](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/severity-info.png) Complete the task associated to this TODO comment. [![rule](https://raw.githubusercontent.com/SonarCommunity/sonar-github/master/images/rule.png)](https://garage.netty.io/sonarqube/coding_rules#rule_key=squid%3AS1135)\n","created_at":"2016-10-12T15:19:59Z","updated_at":"2016-10-12T15:19:59Z","html_url":"https://github.com/netty/netty/pull/5914#discussion_r83029617","pull_request_url":"https://api.github.com/repos/netty/netty/pulls/5914","_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/83029617"},"html":{"href":"https://github.com/netty/netty/pull/5914#discussion_r83029617"},"pull_request":{"href":"https://api.github.com/repos/netty/netty/pulls/5914"}}},"pull_request":{"url":"https://api.github.com/repos/netty/netty/pulls/5914","id":88980720,"html_url":"https://github.com/netty/netty/pull/5914","diff_url":"https://github.com/netty/netty/pull/5914.diff","patch_url":"https://github.com/netty/netty/pull/5914.patch","issue_url":"https://api.github.com/repos/netty/netty/issues/5914","number":5914,"state":"open","locked":false,"title":"HTTP/2 Child Channel and FrameCodec Feature Parity.","user":{"login":"buchgr","id":1388179,"avatar_url":"https://avatars.githubusercontent.com/u/1388179?v=3","gravatar_id":"","url":"https://api.github.com/users/buchgr","html_url":"https://github.com/buchgr","followers_url":"https://api.github.com/users/buchgr/followers","following_url":"https://api.github.com/users/buchgr/following{/other_user}","gists_url":"https://api.github.com/users/buchgr/gists{/gist_id}","starred_url":"https://api.github.com/users/buchgr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/buchgr/subscriptions","organizations_url":"https://api.github.com/users/buchgr/orgs","repos_url":"https://api.github.com/users/buchgr/repos","events_url":"https://api.github.com/users/buchgr/events{/privacy}","received_events_url":"https://api.github.com/users/buchgr/received_events","type":"User","site_admin":false},"body":"First of all, I would like to say that I am really sorry for the large PR. It's really 2 PRs in one, but unfortunately it's been some time after the first PR #5747, performance evaluation #5772 and moving gRPC to this API, in which things have changed so much that it was impossible for me to keep the changes separate and in sync :(.\r\n\r\nThis PR firstly adds outbound flow control to the `Http2MultiplexCodec` and secondly aims to make the `Http2FrameCodec` ready to be used standalone and without the `Http2MultiplexCodec`. I have already (experimentally) ported gRPC to this `Http2FrameCodec` API, with good results.\r\n\r\nClasses like `Http2Stream2`, `Http2Stream2Exception`, `Http2Stream2Visitor`, ... are planned to eventually replace their counterparts without the 2 suffix. That is, once we have a plan to make the Http2Connection API package private.\r\n\r\n---------------------\r\n**Motivation:**\r\n\r\nThis PR (unfortunately) does 4 things:\r\n\r\n1. Add outbound flow control to the Http2MultiplexCodec: \r\nThe HTTP/2 child channel API should interact with HTTP/2 outbound/remote flow control. That is, if a H2 stream used up all its flow control window, the corresponding child channel should be marked unwritable and a writability-changed event should be fired. Similarly, a unwritable child channel should be marked writable and a writability-event should be fired, once a WINDOW_UPDATE frame has been received. The changes are (mostly) contained in ChannelOutboundBuffer, AbstractHttp2StreamChannel and Http2MultiplexCodec.\r\n2. Introduce a Http2Stream2 object, that is used instead of stream identifiers on stream frames. A Http2Stream2 object allows an application to attach state to it, and so a application handler no longer needs to maintain stream state (i.e. in a map(id -> state)) himself.\r\n3. Remove stream state events, which are no longer necessary due to the introduction of Http2Stream2. Also those stream state events have been found hard and complex to work with, when porting gRPC to the Http2FrameCodec.\r\n4. Add support for HTTP/2 frames that have not yet been implemented, like PING and SETTINGS. Also add a Http2FrameCodecBuilder that exposes options from the Http2ConnectionHandler API that couldn't else be used with the frame codec, like buffering outbound streams, window update ratio, frame logger, etc.\r\n\r\n**Modifications:**\r\n\r\n1. A child channel's writability and a H2 stream's outbound flow control window interact, as described in the motivation. A channel handler is free to ignore the channel's writability, in which case the parent channel is reponsible for buffering writes until a WINDOW_UPDATE is received. The connection-level flow control window is ignored for now. That is, a child channel's writability is only affected by the stream-level flow control window. So a child channel could be marked writable, even though the connection-level flow control window is zero.\r\n2. Modify Http2StreamFrame and the Http2FrameCodec to take a Http2Stream2 object intstead of a primitive integer. Introduce a special Http2ChannelDuplexHandler that has `newStream()` and `forEachActiveStream()` methods. It's recommended for a user to extend from this handler, to use those advanced features.\r\n3. As explained in the documentation, a new inbound stream active can be detected by checking if the Http2Stream2.managedState() of a Http2HeadersFrame is null. An outbound stream active can be detected by adding a listener to the ChannelPromise of the write of the first Http2HeadersFrame. A stream closed event can be listened to by adding a listener to the Http2Stream2.closeFuture().\r\n4. Add a simple Http2FrameCodecBuilder and implement the missing frame types.\r\n\r\n**Result:**\r\n\r\n1. The Http2MultiplexCodec supports outbound flow control.\r\n2. The Http2FrameCodec API makes it easy for a user to manage custom stream specific state and to create new outbound streams.\r\n3. The Http2FrameCodec API is much cleaner and easier to work with. Hacks like the `ChannelCarryingHeadersFrame` are no longer necessary.\r\n4. The Http2FrameCodec now also supports PING and SETTINGS frames. The `Http2FrameCodecBuilder` allows the `Http2FrameCodec` to use some of the rich features of the `Http2ConnectionHandler` API.","created_at":"2016-10-12T09:20:57Z","updated_at":"2016-10-12T15:19:59Z","closed_at":null,"merged_at":null,"merge_commit_sha":"936858aa1417d0503205abebcb6870713a62b0e5","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/netty/netty/pulls/5914/commits","review_comments_url":"https://api.github.com/repos/netty/netty/pulls/5914/comments","review_comment_url":"https://api.github.com/repos/netty/netty/pulls/comments{/number}","comments_url":"https://api.github.com/repos/netty/netty/issues/5914/comments","statuses_url":"https://api.github.com/repos/netty/netty/statuses/cab93db7d7232cdc2602491454e374324ca86c64","head":{"label":"buchgr:h2framecodec_feature_parity","ref":"h2framecodec_feature_parity","sha":"cab93db7d7232cdc2602491454e374324ca86c64","user":{"login":"buchgr","id":1388179,"avatar_url":"https://avatars.githubusercontent.com/u/1388179?v=3","gravatar_id":"","url":"https://api.github.com/users/buchgr","html_url":"https://github.com/buchgr","followers_url":"https://api.github.com/users/buchgr/followers","following_url":"https://api.github.com/users/buchgr/following{/other_user}","gists_url":"https://api.github.com/users/buchgr/gists{/gist_id}","starred_url":"https://api.github.com/users/buchgr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/buchgr/subscriptions","organizations_url":"https://api.github.com/users/buchgr/orgs","repos_url":"https://api.github.com/users/buchgr/repos","events_url":"https://api.github.com/users/buchgr/events{/privacy}","received_events_url":"https://api.github.com/users/buchgr/received_events","type":"User","site_admin":false},"repo":{"id":17233219,"name":"netty","full_name":"buchgr/netty","owner":{"login":"buchgr","id":1388179,"avatar_url":"https://avatars.githubusercontent.com/u/1388179?v=3","gravatar_id":"","url":"https://api.github.com/users/buchgr","html_url":"https://github.com/buchgr","followers_url":"https://api.github.com/users/buchgr/followers","following_url":"https://api.github.com/users/buchgr/following{/other_user}","gists_url":"https://api.github.com/users/buchgr/gists{/gist_id}","starred_url":"https://api.github.com/users/buchgr/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/buchgr/subscriptions","organizations_url":"https://api.github.com/users/buchgr/orgs","repos_url":"https://api.github.com/users/buchgr/repos","events_url":"https://api.github.com/users/buchgr/events{/privacy}","received_events_url":"https://api.github.com/users/buchgr/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/buchgr/netty","description":"Netty project - an event-driven asynchronous network application framework","fork":true,"url":"https://api.github.com/repos/buchgr/netty","forks_url":"https://api.github.com/repos/buchgr/netty/forks","keys_url":"https://api.github.com/repos/buchgr/netty/keys{/key_id}","collaborators_url":"https://api.github.com/repos/buchgr/netty/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/buchgr/netty/teams","hooks_url":"https://api.github.com/repos/buchgr/netty/hooks","issue_events_url":"https://api.github.com/repos/buchgr/netty/issues/events{/number}","events_url":"https://api.github.com/repos/buchgr/netty/events","assignees_url":"https://api.github.com/repos/buchgr/netty/assignees{/user}","branches_url":"https://api.github.com/repos/buchgr/netty/branches{/branch}","tags_url":"https://api.github.com/repos/buchgr/netty/tags","blobs_url":"https://api.github.com/repos/buchgr/netty/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/buchgr/netty/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/buchgr/netty/git/refs{/sha}","trees_url":"https://api.github.com/repos/buchgr/netty/git/trees{/sha}","statuses_url":"https://api.github.com/repos/buchgr/netty/statuses/{sha}","languages_url":"https://api.github.com/repos/buchgr/netty/languages","stargazers_url":"https://api.github.com/repos/buchgr/netty/stargazers","contributors_url":"https://api.github.com/repos/buchgr/netty/contributors","subscribers_url":"https://api.github.com/repos/buchgr/netty/subscribers","subscription_url":"https://api.github.com/repos/buchgr/netty/subscription","commits_url":"https://api.github.com/repos/buchgr/netty/commits{/sha}","git_commits_url":"https://api.github.com/repos/buchgr/netty/git/commits{/sha}","comments_url":"https://api.github.com/repos/buchgr/netty/comments{/number}","issue_comment_url":"https://api.github.com/repos/buchgr/netty/issues/comments{/number}","contents_url":"https://api.github.com/repos/buchgr/netty/contents/{+path}","compare_url":"https://api.github.com/repos/buchgr/netty/compare/{base}...{head}","merges_url":"https://api.github.com/repos/buchgr/netty/merges","archive_url":"https://api.github.com/repos/buchgr/netty/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/buchgr/netty/downloads","issues_url":"https://api.github.com/repos/buchgr/netty/issues{/number}","pulls_url":"https://api.github.com/repos/buchgr/netty/pulls{/number}","milestones_url":"https://api.github.com/repos/buchgr/netty/milestones{/number}","notifications_url":"https://api.github.com/repos/buchgr/netty/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/buchgr/netty/labels{/name}","releases_url":"https://api.github.com/repos/buchgr/netty/releases{/id}","deployments_url":"https://api.github.com/repos/buchgr/netty/deployments","created_at":"2014-02-27T02:09:39Z","updated_at":"2015-03-10T20:55:16Z","pushed_at":"2016-10-12T13:35:12Z","git_url":"git://github.com/buchgr/netty.git","ssh_url":"git@github.com:buchgr/netty.git","clone_url":"https://github.com/buchgr/netty.git","svn_url":"https://github.com/buchgr/netty","homepage":"http://netty.io/","size":38585,"stargazers_count":1,"watchers_count":1,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":3,"mirror_url":null,"open_issues_count":0,"forks":3,"open_issues":0,"watchers":1,"default_branch":"master"}},"base":{"label":"netty:4.1","ref":"4.1","sha":"97bf3c0a9bcd6e3f7f9ad5465779242c0edde390","user":{"login":"netty","id":473791,"avatar_url":"https://avatars.githubusercontent.com/u/473791?v=3","gravatar_id":"","url":"https://api.github.com/users/netty","html_url":"https://github.com/netty","followers_url":"https://api.github.com/users/netty/followers","following_url":"https://api.github.com/users/netty/following{/other_user}","gists_url":"https://api.github.com/users/netty/gists{/gist_id}","starred_url":"https://api.github.com/users/netty/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/netty/subscriptions","organizations_url":"https://api.github.com/users/netty/orgs","repos_url":"https://api.github.com/users/netty/repos","events_url":"https://api.github.com/users/netty/events{/privacy}","received_events_url":"https://api.github.com/users/netty/received_events","type":"Organization","site_admin":false},"repo":{"id":1064563,"name":"netty","full_name":"netty/netty","owner":{"login":"netty","id":473791,"avatar_url":"https://avatars.githubusercontent.com/u/473791?v=3","gravatar_id":"","url":"https://api.github.com/users/netty","html_url":"https://github.com/netty","followers_url":"https://api.github.com/users/netty/followers","following_url":"https://api.github.com/users/netty/following{/other_user}","gists_url":"https://api.github.com/users/netty/gists{/gist_id}","starred_url":"https://api.github.com/users/netty/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/netty/subscriptions","organizations_url":"https://api.github.com/users/netty/orgs","repos_url":"https://api.github.com/users/netty/repos","events_url":"https://api.github.com/users/netty/events{/privacy}","received_events_url":"https://api.github.com/users/netty/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/netty/netty","description":"Netty project - an event-driven asynchronous network application framework","fork":false,"url":"https://api.github.com/repos/netty/netty","forks_url":"https://api.github.com/repos/netty/netty/forks","keys_url":"https://api.github.com/repos/netty/netty/keys{/key_id}","collaborators_url":"https://api.github.com/repos/netty/netty/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/netty/netty/teams","hooks_url":"https://api.github.com/repos/netty/netty/hooks","issue_events_url":"https://api.github.com/repos/netty/netty/issues/events{/number}","events_url":"https://api.github.com/repos/netty/netty/events","assignees_url":"https://api.github.com/repos/netty/netty/assignees{/user}","branches_url":"https://api.github.com/repos/netty/netty/branches{/branch}","tags_url":"https://api.github.com/repos/netty/netty/tags","blobs_url":"https://api.github.com/repos/netty/netty/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/netty/netty/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/netty/netty/git/refs{/sha}","trees_url":"https://api.github.com/repos/netty/netty/git/trees{/sha}","statuses_url":"https://api.github.com/repos/netty/netty/statuses/{sha}","languages_url":"https://api.github.com/repos/netty/netty/languages","stargazers_url":"https://api.github.com/repos/netty/netty/stargazers","contributors_url":"https://api.github.com/repos/netty/netty/contributors","subscribers_url":"https://api.github.com/repos/netty/netty/subscribers","subscription_url":"https://api.github.com/repos/netty/netty/subscription","commits_url":"https://api.github.com/repos/netty/netty/commits{/sha}","git_commits_url":"https://api.github.com/repos/netty/netty/git/commits{/sha}","comments_url":"https://api.github.com/repos/netty/netty/comments{/number}","issue_comment_url":"https://api.github.com/repos/netty/netty/issues/comments{/number}","contents_url":"https://api.github.com/repos/netty/netty/contents/{+path}","compare_url":"https://api.github.com/repos/netty/netty/compare/{base}...{head}","merges_url":"https://api.github.com/repos/netty/netty/merges","archive_url":"https://api.github.com/repos/netty/netty/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/netty/netty/downloads","issues_url":"https://api.github.com/repos/netty/netty/issues{/number}","pulls_url":"https://api.github.com/repos/netty/netty/pulls{/number}","milestones_url":"https://api.github.com/repos/netty/netty/milestones{/number}","notifications_url":"https://api.github.com/repos/netty/netty/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/netty/netty/labels{/name}","releases_url":"https://api.github.com/repos/netty/netty/releases{/id}","deployments_url":"https://api.github.com/repos/netty/netty/deployments","created_at":"2010-11-09T09:22:21Z","updated_at":"2016-10-12T15:15:40Z","pushed_at":"2016-10-12T13:35:13Z","git_url":"git://github.com/netty/netty.git","ssh_url":"git@github.com:netty/netty.git","clone_url":"https://github.com/netty/netty.git","svn_url":"https://github.com/netty/netty","homepage":"http://netty.io/","size":72759,"stargazers_count":7980,"watchers_count":7980,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":4013,"mirror_url":null,"open_issues_count":325,"forks":4013,"open_issues":325,"watchers":7980,"default_branch":"4.1"}},"_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/5914"},"html":{"href":"https://github.com/netty/netty/pull/5914"},"issue":{"href":"https://api.github.com/repos/netty/netty/issues/5914"},"comments":{"href":"https://api.github.com/repos/netty/netty/issues/5914/comments"},"review_comments":{"href":"https://api.github.com/repos/netty/netty/pulls/5914/comments"},"review_comment":{"href":"https://api.github.com/repos/netty/netty/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/netty/netty/pulls/5914/commits"},"statuses":{"href":"https://api.github.com/repos/netty/netty/statuses/cab93db7d7232cdc2602491454e374324ca86c64"}}}}
{"actor":{"display_login":"netkins"}}