GitHub PullRequestReviewCommentEvent at 2014-10-24 18:52:55
Repository: netty
Actor: fredericBregier
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/netty/netty/pulls/comments/19357194","id":19357194,"diff_hunk":"@@ -121,58 +137,96 @@ public void handlerAdded(ChannelHandlerContext ctx) throws Exception {\n     }\n \n     @Override\n-    public synchronized void handlerRemoved(ChannelHandlerContext ctx) throws Exception {\n-        if (trafficCounter != null) {\n-            trafficCounter.stop();\n-        }\n-        for (ToSend toSend : messagesQueue) {\n-            if (toSend.toSend instanceof ByteBuf) {\n-                ((ByteBuf) toSend.toSend).release();\n+    public void handlerRemoved(ChannelHandlerContext ctx) throws Exception {\n+        trafficCounter.stop();\n+        // write order control\n+        synchronized (this) {\n+            if (ctx.channel().isActive()) {\n+                for (ToSend toSend : messagesQueue) {\n+                    long size = calculateSize(toSend.toSend);\n+                    trafficCounter.bytesRealWriteFlowControl(size);\n+                    queueSize -= size;\n+                    ctx.write(toSend.toSend, toSend.promise);","path":"handler/src/main/java/io/netty/handler/traffic/ChannelTrafficShapingHandler.java","position":90,"original_position":90,"commit_id":"743484db69a58955753df7b14bcd836ce2c8dfa4","original_commit_id":"743484db69a58955753df7b14bcd836ce2c8dfa4","user":{"login":"fredericBregier","id":477131,"avatar_url":"https://avatars.githubusercontent.com/u/477131?v=2","gravatar_id":"","url":"https://api.github.com/users/fredericBregier","html_url":"https://github.com/fredericBregier","followers_url":"https://api.github.com/users/fredericBregier/followers","following_url":"https://api.github.com/users/fredericBregier/following{/other_user}","gists_url":"https://api.github.com/users/fredericBregier/gists{/gist_id}","starred_url":"https://api.github.com/users/fredericBregier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fredericBregier/subscriptions","organizations_url":"https://api.github.com/users/fredericBregier/orgs","repos_url":"https://api.github.com/users/fredericBregier/repos","events_url":"https://api.github.com/users/fredericBregier/events{/privacy}","received_events_url":"https://api.github.com/users/fredericBregier/received_events","type":"User","site_admin":false},"body":"The write order is per channel only, not globally, so the reason there is a queue per channel in GlobalTSH, and one queue only in ChannelTSH.","created_at":"2014-10-24T18:52:55Z","updated_at":"2014-10-24T18:52:55Z","html_url":"https://github.com/netty/netty/pull/3037#discussion_r19357194","pull_request_url":"https://api.github.com/repos/netty/netty/pulls/3037","_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/19357194"},"html":{"href":"https://github.com/netty/netty/pull/3037#discussion_r19357194"},"pull_request":{"href":"https://api.github.com/repos/netty/netty/pulls/3037"}}},"pull_request":{"url":"https://api.github.com/repos/netty/netty/pulls/3037","id":23152434,"html_url":"https://github.com/netty/netty/pull/3037","diff_url":"https://github.com/netty/netty/pull/3037.diff","patch_url":"https://github.com/netty/netty/pull/3037.patch","issue_url":"https://api.github.com/repos/netty/netty/issues/3037","number":3037,"state":"open","locked":false,"title":"Big transfer and Write traffic shaping issues fixed","user":{"login":"fredericBregier","id":477131,"avatar_url":"https://avatars.githubusercontent.com/u/477131?v=2","gravatar_id":"","url":"https://api.github.com/users/fredericBregier","html_url":"https://github.com/fredericBregier","followers_url":"https://api.github.com/users/fredericBregier/followers","following_url":"https://api.github.com/users/fredericBregier/following{/other_user}","gists_url":"https://api.github.com/users/fredericBregier/gists{/gist_id}","starred_url":"https://api.github.com/users/fredericBregier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fredericBregier/subscriptions","organizations_url":"https://api.github.com/users/fredericBregier/orgs","repos_url":"https://api.github.com/users/fredericBregier/repos","events_url":"https://api.github.com/users/fredericBregier/events{/privacy}","received_events_url":"https://api.github.com/users/fredericBregier/received_events","type":"User","site_admin":false},"body":"Motivation:\r\nSeveral issues were shown by various ticket (#2900 #2956).\r\nAlso use the improvement on writability user management from #3036.\r\n\r\nIssue #2900\r\nWhen a huge amount of data are written, the current behavior of the\r\nTrafficShaping handler is to limit the delay to 15s, whatever the delay\r\nthe previous write has. This is wrong, and when a huge amount of writes\r\nare done in a short time, the traffic is not correctly shapened.\r\n\r\nMoreover, there is a high risk of OOM if one is not using in his/her own\r\nhandler for instance ChannelFuture.addListener() to handle the write\r\nbufferisation in the TrafficShapingHandler.\r\n\r\nThis fix use the \"user-defined writability flags\" from #3036 to\r\nallow the TrafficShapingHandlers to \"user-defined\" managed writability directly, as\r\nfor reading, thus using the default isWritable() and\r\nchannelWritabilityChanged().\r\nThis allows for instance HttpChunkedInput to be fully compatible.\r\n\r\nThe \"bandwidth\" compute on write is only on \"acquired\" write orders, not\r\non \"real\" write orders, which is wrong from statistic point of view.\r\n\r\nIssue #2956\r\nWhen using GlobalTrafficShaping, every write (and read) are\r\nsynchronized, thus leading to a drop of performance.\r\nChannelTrafficShaping is not touched by this issue since synchronized is\r\nthen correct (handler is per channel, so the synchronized).\r\n\r\nModifications:\r\nThe current write delay computation takes into account the previous\r\nwrite delay and time to check is the 15s delay (maxTime) is really\r\nexceeded or not (using last scheduled write time). The algorithm is\r\nsimplified and in the same time more accurate.\r\n\r\nThis proposal uses the #3036 improvement on user-defined writability flags.\r\n\r\nWhen the real write occurs, the statistics are update accordingly on a\r\nnew attribute (getRealWriteThroughput()).\r\n\r\nTo limit the synchronisations, all synchronized on\r\nGlobalTrafficShapingHandler on submitWrite were removed. They are\r\nreplaced with a lock per channel (since synchronization is still needed\r\nto prevent unordered write per channel), as in the sendAllValid method\r\nfor the very same reason.\r\nAlso all synchronized on TrafficCounter on read/writeTimeToWait() are\r\nremoved as they are unnecessary since already locked before by the\r\ncaller.\r\nStill the creation and remove operations on lock per channel (PerChannel\r\nobject) are synchronized to prevent concurrency issue on this critical\r\npart, but then limited.\r\n\r\nResult:\r\nThe traffic shaping is more stable, even with a huge number of writes in\r\nshort time by taking into consideration last scheduled write time.\r\n\r\nThe current implementation of TrafficShapingHandler using user-defined writability flags\r\nand default isWritable() and fireChannelWritabilityChanged works as expected.\r\n\r\nThe statistics are more valuable (asked write vs real write).\r\n\r\nVarious examples are shown in one added example (example/traffic), and\r\nin particular how a user handler might managed the write to prevent OOM:\r\n\r\n- using the ChannelFuture.addListener(new GenericFutureListener>() one by one, or by set of writes\r\n- using the ctx.channel().isWritable() and channelWritabilityChanged()\r\n- using a code as HttpChunkedInput to send the chunks according to the channelWritabilityChanged() (softWritability is needed)\r\n- using no specific check, but with the risk of OOM (no control on the bufferisation made by the TSH)\r\n\r\nThe Global TrafficShapingHandler should now have less \"global\"\r\nsynchronization, hoping to the minimum, but still per Channel as needed.\r\n","created_at":"2014-10-22T12:05:59Z","updated_at":"2014-10-24T18:52:55Z","closed_at":null,"merged_at":null,"merge_commit_sha":"5f3d8a90219e7be25ce5aa920f3dc4e0f210cf5c","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/netty/netty/pulls/3037/commits","review_comments_url":"https://api.github.com/repos/netty/netty/pulls/3037/comments","review_comment_url":"https://api.github.com/repos/netty/netty/pulls/comments/{number}","comments_url":"https://api.github.com/repos/netty/netty/issues/3037/comments","statuses_url":"https://api.github.com/repos/netty/netty/statuses/743484db69a58955753df7b14bcd836ce2c8dfa4","head":{"label":"fredericBregier:master-traffic-shaping-write-fix","ref":"master-traffic-shaping-write-fix","sha":"743484db69a58955753df7b14bcd836ce2c8dfa4","user":{"login":"fredericBregier","id":477131,"avatar_url":"https://avatars.githubusercontent.com/u/477131?v=2","gravatar_id":"","url":"https://api.github.com/users/fredericBregier","html_url":"https://github.com/fredericBregier","followers_url":"https://api.github.com/users/fredericBregier/followers","following_url":"https://api.github.com/users/fredericBregier/following{/other_user}","gists_url":"https://api.github.com/users/fredericBregier/gists{/gist_id}","starred_url":"https://api.github.com/users/fredericBregier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fredericBregier/subscriptions","organizations_url":"https://api.github.com/users/fredericBregier/orgs","repos_url":"https://api.github.com/users/fredericBregier/repos","events_url":"https://api.github.com/users/fredericBregier/events{/privacy}","received_events_url":"https://api.github.com/users/fredericBregier/received_events","type":"User","site_admin":false},"repo":{"id":20618860,"name":"netty","full_name":"fredericBregier/netty","owner":{"login":"fredericBregier","id":477131,"avatar_url":"https://avatars.githubusercontent.com/u/477131?v=2","gravatar_id":"","url":"https://api.github.com/users/fredericBregier","html_url":"https://github.com/fredericBregier","followers_url":"https://api.github.com/users/fredericBregier/followers","following_url":"https://api.github.com/users/fredericBregier/following{/other_user}","gists_url":"https://api.github.com/users/fredericBregier/gists{/gist_id}","starred_url":"https://api.github.com/users/fredericBregier/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fredericBregier/subscriptions","organizations_url":"https://api.github.com/users/fredericBregier/orgs","repos_url":"https://api.github.com/users/fredericBregier/repos","events_url":"https://api.github.com/users/fredericBregier/events{/privacy}","received_events_url":"https://api.github.com/users/fredericBregier/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/fredericBregier/netty","description":"Netty project - an event-driven asynchronous network application framework","fork":true,"url":"https://api.github.com/repos/fredericBregier/netty","forks_url":"https://api.github.com/repos/fredericBregier/netty/forks","keys_url":"https://api.github.com/repos/fredericBregier/netty/keys{/key_id}","collaborators_url":"https://api.github.com/repos/fredericBregier/netty/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/fredericBregier/netty/teams","hooks_url":"https://api.github.com/repos/fredericBregier/netty/hooks","issue_events_url":"https://api.github.com/repos/fredericBregier/netty/issues/events{/number}","events_url":"https://api.github.com/repos/fredericBregier/netty/events","assignees_url":"https://api.github.com/repos/fredericBregier/netty/assignees{/user}","branches_url":"https://api.github.com/repos/fredericBregier/netty/branches{/branch}","tags_url":"https://api.github.com/repos/fredericBregier/netty/tags","blobs_url":"https://api.github.com/repos/fredericBregier/netty/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/fredericBregier/netty/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/fredericBregier/netty/git/refs{/sha}","trees_url":"https://api.github.com/repos/fredericBregier/netty/git/trees{/sha}","statuses_url":"https://api.github.com/repos/fredericBregier/netty/statuses/{sha}","languages_url":"https://api.github.com/repos/fredericBregier/netty/languages","stargazers_url":"https://api.github.com/repos/fredericBregier/netty/stargazers","contributors_url":"https://api.github.com/repos/fredericBregier/netty/contributors","subscribers_url":"https://api.github.com/repos/fredericBregier/netty/subscribers","subscription_url":"https://api.github.com/repos/fredericBregier/netty/subscription","commits_url":"https://api.github.com/repos/fredericBregier/netty/commits{/sha}","git_commits_url":"https://api.github.com/repos/fredericBregier/netty/git/commits{/sha}","comments_url":"https://api.github.com/repos/fredericBregier/netty/comments{/number}","issue_comment_url":"https://api.github.com/repos/fredericBregier/netty/issues/comments/{number}","contents_url":"https://api.github.com/repos/fredericBregier/netty/contents/{+path}","compare_url":"https://api.github.com/repos/fredericBregier/netty/compare/{base}...{head}","merges_url":"https://api.github.com/repos/fredericBregier/netty/merges","archive_url":"https://api.github.com/repos/fredericBregier/netty/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/fredericBregier/netty/downloads","issues_url":"https://api.github.com/repos/fredericBregier/netty/issues{/number}","pulls_url":"https://api.github.com/repos/fredericBregier/netty/pulls{/number}","milestones_url":"https://api.github.com/repos/fredericBregier/netty/milestones{/number}","notifications_url":"https://api.github.com/repos/fredericBregier/netty/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/fredericBregier/netty/labels{/name}","releases_url":"https://api.github.com/repos/fredericBregier/netty/releases{/id}","created_at":"2014-06-08T14:54:54Z","updated_at":"2014-08-13T00:02:36Z","pushed_at":"2014-10-24T18:16:00Z","git_url":"git://github.com/fredericBregier/netty.git","ssh_url":"git@github.com:fredericBregier/netty.git","clone_url":"https://github.com/fredericBregier/netty.git","svn_url":"https://github.com/fredericBregier/netty","homepage":"http://netty.io/","size":37591,"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:user_defined_writability","ref":"user_defined_writability","sha":"37cd536402e27c4426211844d3804cc3b92e9a4c","user":{"login":"netty","id":473791,"avatar_url":"https://avatars.githubusercontent.com/u/473791?v=2","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=2","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-10-24T08:02:45Z","pushed_at":"2014-10-22T13:26:41Z","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":119851,"stargazers_count":3658,"watchers_count":3658,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1737,"mirror_url":null,"open_issues_count":169,"forks":1737,"open_issues":169,"watchers":3658,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/3037"},"html":{"href":"https://github.com/netty/netty/pull/3037"},"issue":{"href":"https://api.github.com/repos/netty/netty/issues/3037"},"comments":{"href":"https://api.github.com/repos/netty/netty/issues/3037/comments"},"review_comments":{"href":"https://api.github.com/repos/netty/netty/pulls/3037/comments"},"review_comment":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/netty/netty/pulls/3037/commits"},"statuses":{"href":"https://api.github.com/repos/netty/netty/statuses/743484db69a58955753df7b14bcd836ce2c8dfa4"}}}}