GitHub PullRequestReviewCommentEvent at 2014-11-18 17:59:24
Repository: netty
Actor: Scottmitch
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/netty/netty/pulls/comments/20523958","id":20523958,"diff_hunk":"@@ -63,47 +63,51 @@ public DelegatingDecompressorFrameListener(Http2Connection connection, Http2Fram\n \n     @Override\n     public int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int padding, boolean endOfStream)\n-                    throws Http2Exception {\n+            throws Http2Exception {\n         final Http2Stream stream = connection.stream(streamId);\n-        final EmbeddedChannel decompressor = stream == null ? null : stream.decompressor();\n+        final Http2Decompressor decompressor = stream == null ? null :\n+            (Http2Decompressor) stream.dataGet(Http2Decompressor.class);\n         if (decompressor == null) {\n             // The decompressor may be null if no compatible encoding type was found in this stream's headers\n             return listener.onDataRead(ctx, streamId, data, padding, endOfStream);\n         }\n \n-        // When decompressing, always opt-out of application-level flow control.\n-        // TODO: investigate how to apply application-level flow control when decompressing.\n-        int processedBytes = data.readableBytes() + padding;\n-        try {\n-            // call retain here as it will call release after its written to the channel\n-            decompressor.writeInbound(data.retain());\n-            ByteBuf buf = nextReadableBuf(decompressor);\n-            if (buf == null) {\n-                if (endOfStream) {\n-                    listener.onDataRead(ctx, streamId, Unpooled.EMPTY_BUFFER, padding, true);\n-                }\n-                // END_STREAM is not set and the data could not be decoded yet.\n-                // The assumption has to be there will be more data frames to complete the decode.\n-                // We don't have enough information here to know if this is an error.\n-            } else {\n-                for (;;) {\n-                    final ByteBuf nextBuf = nextReadableBuf(decompressor);\n-                    final boolean endOfStreamForBuf = nextBuf == null && endOfStream;\n-\n-                    listener.onDataRead(ctx, streamId, buf, padding, endOfStreamForBuf);\n-                    if (nextBuf == null) {\n-                        break;\n-                    }\n-\n-                    buf = nextBuf;\n-                }\n-            }\n-            return processedBytes;\n-        } finally {\n+        final EmbeddedChannel channel = decompressor.decompressor();\n+        final int compressedBytes = data.readableBytes() + padding;\n+        int processedBytes = 0;\n+        decompressor.incrementCompressedBytes(compressedBytes);\n+        // call retain here as it will call release after its written to the channel\n+        channel.writeInbound(data.retain());\n+        ByteBuf buf = nextReadableBuf(channel);\n+        if (buf == null) {\n             if (endOfStream) {\n-                cleanup(stream, decompressor);\n+                listener.onDataRead(ctx, streamId, Unpooled.EMPTY_BUFFER, padding, true);\n+            }\n+            // Since no data was decoded the best we can do is consider that all compressed bytes were consumed","path":"codec-http2/src/main/java/io/netty/handler/codec/http2/DelegatingDecompressorFrameListener.java","position":65,"original_position":65,"commit_id":"4ae312ed7656efa6a62631e6a1dac56661941853","original_commit_id":"4ae312ed7656efa6a62631e6a1dac56661941853","user":{"login":"Scottmitch","id":7562868,"avatar_url":"https://avatars.githubusercontent.com/u/7562868?v=3","gravatar_id":"","url":"https://api.github.com/users/Scottmitch","html_url":"https://github.com/Scottmitch","followers_url":"https://api.github.com/users/Scottmitch/followers","following_url":"https://api.github.com/users/Scottmitch/following{/other_user}","gists_url":"https://api.github.com/users/Scottmitch/gists{/gist_id}","starred_url":"https://api.github.com/users/Scottmitch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Scottmitch/subscriptions","organizations_url":"https://api.github.com/users/Scottmitch/orgs","repos_url":"https://api.github.com/users/Scottmitch/repos","events_url":"https://api.github.com/users/Scottmitch/events{/privacy}","received_events_url":"https://api.github.com/users/Scottmitch/received_events","type":"User","site_admin":false},"body":"> If no data was decoded it would seem that we would want to return all of the bytes so that we will receive enough to process the next chunk of data.\r\n\r\nThat was what I was trying to communicate.  The next line is `and return all bytes to flow control.`.  Should change the text?\r\n\r\n> I'm not sure what it means to \"consider that all compressed bytes were consumed\". Do you mean that they were passed to the listener?\r\n\r\nNo they were not passed to the listener because nothing was made available to pass to the listener.\r\n","created_at":"2014-11-18T17:59:24Z","updated_at":"2014-11-18T17:59:24Z","html_url":"https://github.com/netty/netty/pull/3144#discussion_r20523958","pull_request_url":"https://api.github.com/repos/netty/netty/pulls/3144","_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/20523958"},"html":{"href":"https://github.com/netty/netty/pull/3144#discussion_r20523958"},"pull_request":{"href":"https://api.github.com/repos/netty/netty/pulls/3144"}}},"pull_request":{"url":"https://api.github.com/repos/netty/netty/pulls/3144","id":24482669,"html_url":"https://github.com/netty/netty/pull/3144","diff_url":"https://github.com/netty/netty/pull/3144.diff","patch_url":"https://github.com/netty/netty/pull/3144.patch","issue_url":"https://api.github.com/repos/netty/netty/issues/3144","number":3144,"state":"open","locked":false,"title":"HTTP/2 Decompress Flow Control","user":{"login":"Scottmitch","id":7562868,"avatar_url":"https://avatars.githubusercontent.com/u/7562868?v=3","gravatar_id":"","url":"https://api.github.com/users/Scottmitch","html_url":"https://github.com/Scottmitch","followers_url":"https://api.github.com/users/Scottmitch/followers","following_url":"https://api.github.com/users/Scottmitch/following{/other_user}","gists_url":"https://api.github.com/users/Scottmitch/gists{/gist_id}","starred_url":"https://api.github.com/users/Scottmitch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Scottmitch/subscriptions","organizations_url":"https://api.github.com/users/Scottmitch/orgs","repos_url":"https://api.github.com/users/Scottmitch/repos","events_url":"https://api.github.com/users/Scottmitch/events{/privacy}","received_events_url":"https://api.github.com/users/Scottmitch/received_events","type":"User","site_admin":false},"body":"Motivation:\r\nThe current decompression frame listener currently opts-out of application level flow control. The application should still be able to control flow control even if decompression is in use.\r\n\r\nModifications:\r\n- DecompressorFrameListener will maintain how many compressed bytes, decompressed bytes, and processed by the listener bytes.  A ratio will be used to translate these values into application level flow control amount.\r\n\r\nResult:\r\nHTTP/2 decompressor delegates the application level flow control to the listener processing the decompressed data.","created_at":"2014-11-15T01:37:41Z","updated_at":"2014-11-18T17:59:24Z","closed_at":null,"merged_at":null,"merge_commit_sha":"5dde2f1dbdca7b46b9ebe6599b93fbc6a0e8584b","assignee":{"login":"Scottmitch","id":7562868,"avatar_url":"https://avatars.githubusercontent.com/u/7562868?v=3","gravatar_id":"","url":"https://api.github.com/users/Scottmitch","html_url":"https://github.com/Scottmitch","followers_url":"https://api.github.com/users/Scottmitch/followers","following_url":"https://api.github.com/users/Scottmitch/following{/other_user}","gists_url":"https://api.github.com/users/Scottmitch/gists{/gist_id}","starred_url":"https://api.github.com/users/Scottmitch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Scottmitch/subscriptions","organizations_url":"https://api.github.com/users/Scottmitch/orgs","repos_url":"https://api.github.com/users/Scottmitch/repos","events_url":"https://api.github.com/users/Scottmitch/events{/privacy}","received_events_url":"https://api.github.com/users/Scottmitch/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/netty/netty/milestones/105","labels_url":"https://api.github.com/repos/netty/netty/milestones/105/labels","id":800277,"number":105,"title":"5.0.0.Alpha3","description":"","creator":{"login":"Scottmitch","id":7562868,"avatar_url":"https://avatars.githubusercontent.com/u/7562868?v=3","gravatar_id":"","url":"https://api.github.com/users/Scottmitch","html_url":"https://github.com/Scottmitch","followers_url":"https://api.github.com/users/Scottmitch/followers","following_url":"https://api.github.com/users/Scottmitch/following{/other_user}","gists_url":"https://api.github.com/users/Scottmitch/gists{/gist_id}","starred_url":"https://api.github.com/users/Scottmitch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Scottmitch/subscriptions","organizations_url":"https://api.github.com/users/Scottmitch/orgs","repos_url":"https://api.github.com/users/Scottmitch/repos","events_url":"https://api.github.com/users/Scottmitch/events{/privacy}","received_events_url":"https://api.github.com/users/Scottmitch/received_events","type":"User","site_admin":false},"open_issues":63,"closed_issues":5,"state":"open","created_at":"2014-09-23T19:48:30Z","updated_at":"2014-11-15T01:37:49Z","due_on":null,"closed_at":null},"commits_url":"https://api.github.com/repos/netty/netty/pulls/3144/commits","review_comments_url":"https://api.github.com/repos/netty/netty/pulls/3144/comments","review_comment_url":"https://api.github.com/repos/netty/netty/pulls/comments/{number}","comments_url":"https://api.github.com/repos/netty/netty/issues/3144/comments","statuses_url":"https://api.github.com/repos/netty/netty/statuses/4ae312ed7656efa6a62631e6a1dac56661941853","head":{"label":"akamai:http2_decompress_flow_control","ref":"http2_decompress_flow_control","sha":"4ae312ed7656efa6a62631e6a1dac56661941853","user":{"login":"akamai","id":7504551,"avatar_url":"https://avatars.githubusercontent.com/u/7504551?v=3","gravatar_id":"","url":"https://api.github.com/users/akamai","html_url":"https://github.com/akamai","followers_url":"https://api.github.com/users/akamai/followers","following_url":"https://api.github.com/users/akamai/following{/other_user}","gists_url":"https://api.github.com/users/akamai/gists{/gist_id}","starred_url":"https://api.github.com/users/akamai/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/akamai/subscriptions","organizations_url":"https://api.github.com/users/akamai/orgs","repos_url":"https://api.github.com/users/akamai/repos","events_url":"https://api.github.com/users/akamai/events{/privacy}","received_events_url":"https://api.github.com/users/akamai/received_events","type":"Organization","site_admin":false},"repo":{"id":22530159,"name":"netty","full_name":"akamai/netty","owner":{"login":"akamai","id":7504551,"avatar_url":"https://avatars.githubusercontent.com/u/7504551?v=3","gravatar_id":"","url":"https://api.github.com/users/akamai","html_url":"https://github.com/akamai","followers_url":"https://api.github.com/users/akamai/followers","following_url":"https://api.github.com/users/akamai/following{/other_user}","gists_url":"https://api.github.com/users/akamai/gists{/gist_id}","starred_url":"https://api.github.com/users/akamai/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/akamai/subscriptions","organizations_url":"https://api.github.com/users/akamai/orgs","repos_url":"https://api.github.com/users/akamai/repos","events_url":"https://api.github.com/users/akamai/events{/privacy}","received_events_url":"https://api.github.com/users/akamai/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/akamai/netty","description":"Netty project - an event-driven asynchronous network application framework","fork":true,"url":"https://api.github.com/repos/akamai/netty","forks_url":"https://api.github.com/repos/akamai/netty/forks","keys_url":"https://api.github.com/repos/akamai/netty/keys{/key_id}","collaborators_url":"https://api.github.com/repos/akamai/netty/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/akamai/netty/teams","hooks_url":"https://api.github.com/repos/akamai/netty/hooks","issue_events_url":"https://api.github.com/repos/akamai/netty/issues/events{/number}","events_url":"https://api.github.com/repos/akamai/netty/events","assignees_url":"https://api.github.com/repos/akamai/netty/assignees{/user}","branches_url":"https://api.github.com/repos/akamai/netty/branches{/branch}","tags_url":"https://api.github.com/repos/akamai/netty/tags","blobs_url":"https://api.github.com/repos/akamai/netty/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/akamai/netty/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/akamai/netty/git/refs{/sha}","trees_url":"https://api.github.com/repos/akamai/netty/git/trees{/sha}","statuses_url":"https://api.github.com/repos/akamai/netty/statuses/{sha}","languages_url":"https://api.github.com/repos/akamai/netty/languages","stargazers_url":"https://api.github.com/repos/akamai/netty/stargazers","contributors_url":"https://api.github.com/repos/akamai/netty/contributors","subscribers_url":"https://api.github.com/repos/akamai/netty/subscribers","subscription_url":"https://api.github.com/repos/akamai/netty/subscription","commits_url":"https://api.github.com/repos/akamai/netty/commits{/sha}","git_commits_url":"https://api.github.com/repos/akamai/netty/git/commits{/sha}","comments_url":"https://api.github.com/repos/akamai/netty/comments{/number}","issue_comment_url":"https://api.github.com/repos/akamai/netty/issues/comments/{number}","contents_url":"https://api.github.com/repos/akamai/netty/contents/{+path}","compare_url":"https://api.github.com/repos/akamai/netty/compare/{base}...{head}","merges_url":"https://api.github.com/repos/akamai/netty/merges","archive_url":"https://api.github.com/repos/akamai/netty/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/akamai/netty/downloads","issues_url":"https://api.github.com/repos/akamai/netty/issues{/number}","pulls_url":"https://api.github.com/repos/akamai/netty/pulls{/number}","milestones_url":"https://api.github.com/repos/akamai/netty/milestones{/number}","notifications_url":"https://api.github.com/repos/akamai/netty/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/akamai/netty/labels{/name}","releases_url":"https://api.github.com/repos/akamai/netty/releases{/id}","created_at":"2014-08-01T21:49:13Z","updated_at":"2014-08-15T17:45:13Z","pushed_at":"2014-11-18T15:29:50Z","git_url":"git://github.com/akamai/netty.git","ssh_url":"git@github.com:akamai/netty.git","clone_url":"https://github.com/akamai/netty.git","svn_url":"https://github.com/akamai/netty","homepage":"http://netty.io/","size":38776,"stargazers_count":0,"watchers_count":0,"language":"Java","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"netty:master","ref":"master","sha":"3868645d7d3e8494a21a4f3348a5ad2d3dcccb2a","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}","created_at":"2010-11-09T09:22:21Z","updated_at":"2014-11-18T15:23:46Z","pushed_at":"2014-11-17T17:10: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":122108,"stargazers_count":3787,"watchers_count":3787,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1802,"mirror_url":null,"open_issues_count":196,"forks":1802,"open_issues":196,"watchers":3787,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/3144"},"html":{"href":"https://github.com/netty/netty/pull/3144"},"issue":{"href":"https://api.github.com/repos/netty/netty/issues/3144"},"comments":{"href":"https://api.github.com/repos/netty/netty/issues/3144/comments"},"review_comments":{"href":"https://api.github.com/repos/netty/netty/pulls/3144/comments"},"review_comment":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/netty/netty/pulls/3144/commits"},"statuses":{"href":"https://api.github.com/repos/netty/netty/statuses/4ae312ed7656efa6a62631e6a1dac56661941853"}}}}