Repository: netty/netty
Issue: Enhancements to AbstractTrafficShapingHandler / TrafficCounter

1. do not sleep  101 ms due to issues with java and windows

see: http://www.javamex.com/tutorials/threads/sleep_issues.shtml

in getTimeToWait change 

return wait;

to:

return (wait/10)*10;
1. do not  "waste" a thread in TrafficCounter. use netty's Timer instead.

-- Ron


Issue comments:
PS: if desired I could provide these changes.

Hi @rzorzorzo 

On the first item, I was really unaware of this ! Thanks for spotting this !

On the second item, I could agree but the idea is to be able to change the timer... (traffic handler could changed during runtime due to special request).
How could we do this ? I suppose that using the Timer utilitye from Netty will bring us to assigning again and again a new thread at every check (which is probably worst than having one specific thread per counter making some sleep) ?
On Counter, we need to have such one thread per Handler (one unique for Global, but if needed, one per Channel).

I would be please if you can propose both, but perhaps in 2 separates proposal to be able to accept one or the other ?

@rzorzorzo 
Since I was on my way to check some other elements, I may have done the first point (see #346).

Sorry if you were on your way... ;-)
I will try to make the same on release 3.5 immediately.

However on second proposal, I let you make some proposals... ;-)

Frederic,

thnx for 1.

concerning 2:
TrafficCounter sleeps practically 100% of the time. During sleep, the thread is not returned to the pool. With 100k connections, therefore 100k channels, we will need with the current solution 100k threads. Not feasible.

Netty's HashedWheelTimer requires a single Thread. The same timer, and therefore thread can be shared by all TrafficCounters. The dynamic change of checkIntervall can be supported as with the current implementation.

Maybe the following code sections will help:

fields:

``` java
    private Timer timer;  // replace executor
    private TimerTask timerTask;  // created once in start()
    volatile private Timeout timeout = null; // used in stop() to cancel the timer
```

Constructor:

``` java
    public TrafficCounter(TrafficShapingHandler trafficShapingHandler,
            Timer timer, String name, long checkInterval) {       
        this.timer = timer;
        ...
       }
```

start method

``` java
 public void start() {
...
                      timerTask = new TimerTask()
                {

                    @Override
                    public void run(Timeout timeout) throws Exception
                    {
                        if (!monitorActive.get()) {
                                        return;
                                   }
                        trafficMonitoring.run();
                                      TrafficCounter.this.timeout = timer.newTimeout(timerTask, checkInterval.get(), TimeUnit.MILLISECONDS);                        
                    }

                };
                timeout = timer.newTimeout(timerTask, checkInterval.get(), TimeUnit.MILLISECONDS);
...
```

stop method

``` java
    public void stop() {
...
              if (timeout != null)
                timeout.cancel();
```

-- Ron

@rzorzorzo @fredericBregier  this makes a lot of sense

@rzorzorzo I will spend some time to your proposal, see if I understand first, then if I can propose a new version once I had tested it.
Thank you !!

@rzorzorzo and @normanmaurer 
I finally make the change as proposed. I tested it intensively. I find no issue (as for now of course).
I even optimize a bit more the traffic handler:
- there were one thread created to unlock traffic from time to time when needed => Timer
- in the original proposal, 2 objects were created: the TimerTask and the internal TrafficCOunter object => only the timer task now

I hope this is what you guess both of you !

See proposal in Issue #348

Cool.. I will review later today.

Thank you @normanmaurer !
And thank you too @rzorzorzo for these proposals!

I believe we can close this issue then.


Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/traffic/AbstractTrafficShapingHandler.java b/handler/src/main/java/io/netty/handler/traffic/AbstractTrafficShapingHandler.java
index 7da4b29a96c..b0c6fcfda95 100644
--- a/handler/src/main/java/io/netty/handler/traffic/AbstractTrafficShapingHandler.java
+++ b/handler/src/main/java/io/netty/handler/traffic/AbstractTrafficShapingHandler.java
@@ -360,7 +360,7 @@ private long getTimeToWait(long limit, long bytes, long lastTime,
             // Time is too short, so just lets continue
             return 0;
         }
-        return bytes * 1000 / limit - interval;
+        return ((bytes * 1000 / limit - interval)/10)*10;
     }
 
     @Override
diff --git a/handler/src/main/java/io/netty/handler/traffic/TrafficCounter.java b/handler/src/main/java/io/netty/handler/traffic/TrafficCounter.java
index 6976ac08c53..d8cfdb3fb67 100644
--- a/handler/src/main/java/io/netty/handler/traffic/TrafficCounter.java
+++ b/handler/src/main/java/io/netty/handler/traffic/TrafficCounter.java
@@ -248,9 +248,10 @@ public TrafficCounter(AbstractTrafficShapingHandler trafficShapingHandler,
      * @param newcheckInterval
      */
     public void configure(long newcheckInterval) {
-        if (checkInterval.get() != newcheckInterval) {
-            checkInterval.set(newcheckInterval);
-            if (newcheckInterval <= 0) {
+        long newInterval = (newcheckInterval/10)*10;
+        if (checkInterval.get() != newInterval) {
+            checkInterval.set(newInterval);
+            if (newInterval <= 0) {
                 stop();
                 // No more active monitoring
                 lastTime.set(System.currentTimeMillis());


Test patch: