GitHub PullRequestReviewCommentEvent at 2014-10-30 10:34:14
Repository: netty
Actor: fredericBregier
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/netty/netty/pulls/comments/19596832","id":19596832,"diff_hunk":"@@ -121,58 +141,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);\n+                }\n+            } else {\n+                for (ToSend toSend : messagesQueue) {\n+                    if (toSend.toSend instanceof ByteBuf) {\n+                        ((ByteBuf) toSend.toSend).release();\n+                    }\n+                }\n             }\n+            messagesQueue.clear();\n         }\n-        messagesQueue.clear();\n+        releaseWriteSuspended(ctx);\n+        releaseReadSuspended(ctx);\n         super.handlerRemoved(ctx);\n     }\n \n     private static final class ToSend {\n-        final long date;\n+        final long relativeTimeAction;\n         final Object toSend;\n         final ChannelPromise promise;\n \n         private ToSend(final long delay, final Object toSend, final ChannelPromise promise) {\n-            this.date = System.currentTimeMillis() + delay;\n+            this.relativeTimeAction = delay;\n             this.toSend = toSend;\n             this.promise = promise;\n         }\n     }\n \n     @Override\n-    protected synchronized void submitWrite(final ChannelHandlerContext ctx, final Object msg, final long delay,\n+    void submitWrite(final ChannelHandlerContext ctx, final Object msg,","path":"handler/src/main/java/io/netty/handler/traffic/ChannelTrafficShapingHandler.java","position":157,"original_position":184,"commit_id":"3f35ccc1fd4075d8601990e685bbb9c01fd4ba16","original_commit_id":"54aa66ae14265afe4d9ee74523ad78daf1b9774a","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":"As it is now implemented in deprecated mode in AbstractTSH, it is done as requested.","created_at":"2014-10-30T10:34:14Z","updated_at":"2014-10-30T10:34:14Z","html_url":"https://github.com/netty/netty/pull/3052#discussion_r19596832","pull_request_url":"https://api.github.com/repos/netty/netty/pulls/3052","_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/19596832"},"html":{"href":"https://github.com/netty/netty/pull/3052#discussion_r19596832"},"pull_request":{"href":"https://api.github.com/repos/netty/netty/pulls/3052"}}},"pull_request":{"url":"https://api.github.com/repos/netty/netty/pulls/3052","id":23338114,"html_url":"https://github.com/netty/netty/pull/3052","diff_url":"https://github.com/netty/netty/pull/3052.diff","patch_url":"https://github.com/netty/netty/pull/3052.patch","issue_url":"https://api.github.com/repos/netty/netty/issues/3052","number":3052,"state":"open","locked":false,"title":"4.0 Big transfer and Write traffic shaping issues","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\r\ndirectly, as for 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\r\nflags.\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\nAdditionnal changes:\r\n1) Use System.nanoTime() instead of System.currentTimeMillis() and\r\nminimize calls\r\n2) Remove / 10 ° 10 since no more sleep usage\r\n3) Use nanoTime instead of currentTime such that time spend is computed,\r\nnot real time clock. Therefore the \"now\" relative time (nanoTime based)\r\nis passed on all sub methods.\r\n4) Take care of removal of the handler to force write all pending writes\r\nand release read too\r\n8) Review Javadoc to explicit:\r\n\r\n- recommandations to take into account isWritable\r\n\r\n- recommandations to provide reasonable message size according to\r\ntraffic shaping limit\r\n\r\n- explicit \"best effort\" traffic shaping behavior when changing\r\nconfiguration dynamically\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\r\nwritability flags and default isWritable() and\r\nfireChannelWritabilityChanged 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\r\nby one, or by set of writes\r\n\r\n- using the ctx.channel().isWritable() and channelWritabilityChanged()\r\n\r\n- using a code as HttpChunkedInput to send the chunks according to the\r\nchannelWritabilityChanged() (softWritability is needed)\r\n\r\n- using no specific check, but with the risk of OOM (no control on the\r\nbufferisation 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.","created_at":"2014-10-25T11:17:40Z","updated_at":"2014-10-30T10:34:14Z","closed_at":null,"merged_at":null,"merge_commit_sha":"0e6f5ad41bb9110f3feb4bcb6de6f18ad7ce9d46","assignee":{"login":"trustin","id":173918,"avatar_url":"https://avatars.githubusercontent.com/u/173918?v=2","gravatar_id":"","url":"https://api.github.com/users/trustin","html_url":"https://github.com/trustin","followers_url":"https://api.github.com/users/trustin/followers","following_url":"https://api.github.com/users/trustin/following{/other_user}","gists_url":"https://api.github.com/users/trustin/gists{/gist_id}","starred_url":"https://api.github.com/users/trustin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/trustin/subscriptions","organizations_url":"https://api.github.com/users/trustin/orgs","repos_url":"https://api.github.com/users/trustin/repos","events_url":"https://api.github.com/users/trustin/events{/privacy}","received_events_url":"https://api.github.com/users/trustin/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/netty/netty/milestones/106","labels_url":"https://api.github.com/repos/netty/netty/milestones/106/labels","id":833917,"number":106,"title":"4.0.25.Final","description":"","creator":{"login":"trustin","id":173918,"avatar_url":"https://avatars.githubusercontent.com/u/173918?v=2","gravatar_id":"","url":"https://api.github.com/users/trustin","html_url":"https://github.com/trustin","followers_url":"https://api.github.com/users/trustin/followers","following_url":"https://api.github.com/users/trustin/following{/other_user}","gists_url":"https://api.github.com/users/trustin/gists{/gist_id}","starred_url":"https://api.github.com/users/trustin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/trustin/subscriptions","organizations_url":"https://api.github.com/users/trustin/orgs","repos_url":"https://api.github.com/users/trustin/repos","events_url":"https://api.github.com/users/trustin/events{/privacy}","received_events_url":"https://api.github.com/users/trustin/received_events","type":"User","site_admin":false},"open_issues":39,"closed_issues":2,"state":"open","created_at":"2014-10-21T07:03:43Z","updated_at":"2014-10-30T09:41:01Z","due_on":null},"commits_url":"https://api.github.com/repos/netty/netty/pulls/3052/commits","review_comments_url":"https://api.github.com/repos/netty/netty/pulls/3052/comments","review_comment_url":"https://api.github.com/repos/netty/netty/pulls/comments/{number}","comments_url":"https://api.github.com/repos/netty/netty/issues/3052/comments","statuses_url":"https://api.github.com/repos/netty/netty/statuses/3f35ccc1fd4075d8601990e685bbb9c01fd4ba16","head":{"label":"fredericBregier:4.0-traffic-shaping-write-fix","ref":"4.0-traffic-shaping-write-fix","sha":"3f35ccc1fd4075d8601990e685bbb9c01fd4ba16","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-30T09:58:48Z","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":37789,"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:4.0","ref":"4.0","sha":"8db8aca1e7bac8a927bc82968291abbb5ade96be","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-30T09:38:40Z","pushed_at":"2014-10-30T09:10:49Z","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":120674,"stargazers_count":3689,"watchers_count":3689,"language":"Java","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1756,"mirror_url":null,"open_issues_count":172,"forks":1756,"open_issues":172,"watchers":3689,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/netty/netty/pulls/3052"},"html":{"href":"https://github.com/netty/netty/pull/3052"},"issue":{"href":"https://api.github.com/repos/netty/netty/issues/3052"},"comments":{"href":"https://api.github.com/repos/netty/netty/issues/3052/comments"},"review_comments":{"href":"https://api.github.com/repos/netty/netty/pulls/3052/comments"},"review_comment":{"href":"https://api.github.com/repos/netty/netty/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/netty/netty/pulls/3052/commits"},"statuses":{"href":"https://api.github.com/repos/netty/netty/statuses/3f35ccc1fd4075d8601990e685bbb9c01fd4ba16"}}}}