DefaultProtocolChainInstanceHandler

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

DefaultProtocolChainInstanceHandler

Erik Svensson-2
A question here:

In DefaultProtocolChainInstanceHandler
the method poll looks like:

    public ProtocolChain poll() {
        ProtocolChain protocolChain = protocolChains.poll();
        if (protocolChain == null){
            protocolChain = new DefaultProtocolChain();
        }
        return protocolChain;
    }

Should there be a 'offer(protocolChain)' here?

    public ProtocolChain poll() {
        ProtocolChain protocolChain = protocolChains.poll();
        if (protocolChain == null){
            protocolChain = new DefaultProtocolChain();
            // PATCH
            offer(protocolChain);
            // END PATCH
        }
        return protocolChain;
    }

Otherwise a new protocolchain will be created everytime poll is called
unless one is manually added through offer.

cheers

Erik Svensson, SIX AB


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Jeanfrancois Arcand-2


Erik Svensson wrote:

> A question here:
>
> In DefaultProtocolChainInstanceHandler
> the method poll looks like:
>
>     public ProtocolChain poll() {
>         ProtocolChain protocolChain = protocolChains.poll();
>         if (protocolChain == null){
>             protocolChain = new DefaultProtocolChain();
>         }
>         return protocolChain;
>     }
>
> Should there be a 'offer(protocolChain)' here?
>
>     public ProtocolChain poll() {
>         ProtocolChain protocolChain = protocolChains.poll();
>         if (protocolChain == null){
>             protocolChain = new DefaultProtocolChain();
>             // PATCH
>             offer(protocolChain);
>             // END PATCH
>         }
>         return protocolChain;
>     }
>
> Otherwise a new protocolchain will be created everytime poll is called
> unless one is manually added through offer.

The contract here is when you do poll, you also invoke the
DefaultProtocolChainInstanceHandler.offer() method. But you found a bug
in Grizzly as the offer method is never called!!

Originally, the offer() method was called...looks like we lost it
between 1.5.x and 1.7.1! Let me re-add it so your stuff can work. Now
Your patch will not work as two thread might ends up with the same
protocol chain, and since the protocol chain are stateful, we will get
data corruption.

Thanks!

-- Jeanfrancois

>
> cheers
>
> Erik Svensson, SIX AB
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Erik Svensson-2
On 2/11/08 4:16 PM, "Jeanfrancois Arcand" <[hidden email]>
wrote:

>
>
> Erik Svensson wrote:
>> A question here:
>>
>> In DefaultProtocolChainInstanceHandler
>> the method poll looks like:
>>
>>     public ProtocolChain poll() {
>>         ProtocolChain protocolChain = protocolChains.poll();
>>         if (protocolChain == null){
>>             protocolChain = new DefaultProtocolChain();
>>         }
>>         return protocolChain;
>>     }
>>
>> Should there be a 'offer(protocolChain)' here?
>>
>>     public ProtocolChain poll() {
>>         ProtocolChain protocolChain = protocolChains.poll();
>>         if (protocolChain == null){
>>             protocolChain = new DefaultProtocolChain();
>>             // PATCH
>>             offer(protocolChain);
>>             // END PATCH
>>         }
>>         return protocolChain;
>>     }
>>
>> Otherwise a new protocolchain will be created everytime poll is called
>> unless one is manually added through offer.
>
> The contract here is when you do poll, you also invoke the
> DefaultProtocolChainInstanceHandler.offer() method. But you found a bug
> in Grizzly as the offer method is never called!!
>
> Originally, the offer() method was called...looks like we lost it
> between 1.5.x and 1.7.1! Let me re-add it so your stuff can work. Now
> Your patch will not work as two thread might ends up with the same
> protocol chain, and since the protocol chain are stateful, we will get
> data corruption.

Yes, I noticed that. I need to think out some other way to cut down the code
needed to set things up before I can give this to my co-developers.
 
cheers
Erik Svensson, SIX AB


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Oleksiy Stashok
Hello,

IMHO to use correctly ProtocolChainInstanceHandler.offer(ProtocolChain)
- we need to add one more method to ProtocolChain API:
ProtocolChainInstanceHandler getInstanceHandler();

as at time, when ProtocolChain could be released - we don't know from
which ProtocolChainInstanceHandler the ProtocolChain was polled from.
Once method above will be added to the ProtocolChain interface - we can
easily implement ProtocolChain releasing in Controller's returnContext()
method:

    public void returnContext(Context ctx){
/        ProtocolChain protocolChain = ctx.getProtocolChain();
        protocolChain.getInstanceHandler().offer(protocolChain);
/        contexts.offer(ctx);
    }

What do you think?

Thanks.

WBR,
Alexey.


Erik Svensson wrote:

> On 2/11/08 4:16 PM, "Jeanfrancois Arcand" <[hidden email]>
> wrote:
>
>  
>> Erik Svensson wrote:
>>    
>>> A question here:
>>>
>>> In DefaultProtocolChainInstanceHandler
>>> the method poll looks like:
>>>
>>>     public ProtocolChain poll() {
>>>         ProtocolChain protocolChain = protocolChains.poll();
>>>         if (protocolChain == null){
>>>             protocolChain = new DefaultProtocolChain();
>>>         }
>>>         return protocolChain;
>>>     }
>>>
>>> Should there be a 'offer(protocolChain)' here?
>>>
>>>     public ProtocolChain poll() {
>>>         ProtocolChain protocolChain = protocolChains.poll();
>>>         if (protocolChain == null){
>>>             protocolChain = new DefaultProtocolChain();
>>>             // PATCH
>>>             offer(protocolChain);
>>>             // END PATCH
>>>         }
>>>         return protocolChain;
>>>     }
>>>
>>> Otherwise a new protocolchain will be created everytime poll is called
>>> unless one is manually added through offer.
>>>      
>> The contract here is when you do poll, you also invoke the
>> DefaultProtocolChainInstanceHandler.offer() method. But you found a bug
>> in Grizzly as the offer method is never called!!
>>
>> Originally, the offer() method was called...looks like we lost it
>> between 1.5.x and 1.7.1! Let me re-add it so your stuff can work. Now
>> Your patch will not work as two thread might ends up with the same
>> protocol chain, and since the protocol chain are stateful, we will get
>> data corruption.
>>    
>
> Yes, I noticed that. I need to think out some other way to cut down the code
> needed to set things up before I can give this to my co-developers.
>  
> cheers
> Erik Svensson, SIX AB
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>  

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Jeanfrancois Arcand-2
Salut,

Oleksiy Stashok wrote:

> Hello,
>
> IMHO to use correctly ProtocolChainInstanceHandler.offer(ProtocolChain)
> - we need to add one more method to ProtocolChain API:
> ProtocolChainInstanceHandler getInstanceHandler();
>
> as at time, when ProtocolChain could be released - we don't know from
> which ProtocolChainInstanceHandler the ProtocolChain was polled from.
> Once method above will be added to the ProtocolChain interface - we can
> easily implement ProtocolChain releasing in Controller's returnContext()
> method:
>
>    public void returnContext(Context ctx){
> /        ProtocolChain protocolChain = ctx.getProtocolChain();
>        protocolChain.getInstanceHandler().offer(protocolChain);
> /        contexts.offer(ctx);
>    }
>
> What do you think?
An alternative is to change the Controller and delegate the
ProtocolChain configuration to the Context (see patch). That way no API
changes :-)

What do you think?

-- Jeanfrancois




Index: src/main/java/com/sun/grizzly/DefaultProtocolChain.java
===================================================================
--- src/main/java/com/sun/grizzly/DefaultProtocolChain.java (revision 784)
+++ src/main/java/com/sun/grizzly/DefaultProtocolChain.java (working copy)
@@ -78,7 +78,6 @@
                 int currentPosition = executeProtocolFilter(ctx);
                 reinvokeChain = postExecuteProtocolFilter(currentPosition, ctx);
             }
-            ctx.recycle();
         }
         controller.returnContext(ctx);
     }
Index: src/main/java/com/sun/grizzly/Controller.java
===================================================================
--- src/main/java/com/sun/grizzly/Controller.java (revision 784)
+++ src/main/java/com/sun/grizzly/Controller.java (working copy)
@@ -360,11 +360,7 @@
                         }
 
                         if (delegateToWorkerThread){
-                            pciHandler = selectorHandler
-                                    .getProtocolChainInstanceHandler();
-                            protocolChain = (pciHandler != null ?
-                                pciHandler.poll(): instanceHandler.poll());
-                            Context context = pollContext(key,protocolChain);
+                            Context context = pollContext(key);
                             context.setSelectorHandler(selectorHandler);
                             context.setPipeline(selectorHandler.pipeline());
                             context.setAsyncQueueReader(
@@ -469,33 +465,20 @@
                     "with known SelectorHandler");
         }
     }
+
     
-    
     /**
      * Get an instance of a <code>Context</code>
      * @param key <code>SelectionKey</code>
      * @return <code>Context</code>
-     */
-    public Context pollContext(SelectionKey key){
-        return pollContext(key,instanceHandler.poll());
-    }
-    
-    /**
-     * Get an instance of a <code>Context</code>
-     * @param key <code>SelectionKey</code>
-     * @param protocolChain The ProtocolChain used to execute the
-     *                      returned Context.
-     * @return <code>Context</code>
      */    
-    protected Context pollContext(SelectionKey key,
-            ProtocolChain protocolChain){
+    public Context pollContext(SelectionKey key){
         Context ctx = contexts.poll();
         if (ctx == null){
             ctx = new Context();
         }
         ctx.setController(this);
         ctx.setSelectionKey(key);
-        ctx.setProtocolChain(protocolChain);
         return ctx;
     }
     
@@ -504,6 +487,14 @@
      * @param ctx - the <code>Context</code>
      */
     public void returnContext(Context ctx){
+        ProtocolChainInstanceHandler pciHandler =
+                ctx.getSelectorHandler().getProtocolChainInstanceHandler();
+        if (pciHandler == null){
+            pciHandler = instanceHandler;
+        }
+        instanceHandler.offer(ctx.getProtocolChain());    
+        
+        ctx.recycle();
         contexts.offer(ctx);
     }
     
Index: src/main/java/com/sun/grizzly/Context.java
===================================================================
--- src/main/java/com/sun/grizzly/Context.java (revision 784)
+++ src/main/java/com/sun/grizzly/Context.java (working copy)
@@ -402,7 +402,14 @@
      *      be executed in separate thread, false - in current thread.
      * @throws com.sun.grizzly.PipelineFullException
      */
-    public void execute(ContextTask contextTask, boolean runInSeparateThread) throws PipelineFullException {
+    public void execute(ContextTask contextTask, boolean runInSeparateThread) throws PipelineFullException {      
+        if (protocolChain == null){
+            ProtocolChainInstanceHandler pciHandler = selectorHandler
+                    .getProtocolChainInstanceHandler();
+            protocolChain = (pciHandler != null ?
+                pciHandler.poll(): controller.getProtocolChainInstanceHandler().poll());  
+        }
+        
         if (contextTask != null) {
             contextTask.setContext(this);
             if (runInSeparateThread) {
Index: src/main/java/com/sun/grizzly/TCPSelectorHandler.java
===================================================================
--- src/main/java/com/sun/grizzly/TCPSelectorHandler.java (revision 784)
+++ src/main/java/com/sun/grizzly/TCPSelectorHandler.java (working copy)
@@ -1071,11 +1071,12 @@
             instanceHandler.poll() :
             serverContext.getController().getProtocolChainInstanceHandler().poll();
         
-        final Context context = serverContext.getController().pollContext(key, protocolChain);
+        final Context context = serverContext.getController().pollContext(key);
         context.setSelectionKey(key);
         context.setSelectorHandler(this);
         context.setAsyncQueueReader(asyncQueueReader);
         context.setAsyncQueueWriter(asyncQueueWriter);
+        context.setProtocolChain(protocolChain);
         return context;
     }
     


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Jeanfrancois Arcand-2
Oups should have been:

>      public void returnContext(Context ctx){
> +        ProtocolChainInstanceHandler pciHandler =
> +                ctx.getSelectorHandler().getProtocolChainInstanceHandler();
> +        if (pciHandler == null){
> +            pciHandler = instanceHandler;
> +        }
> +        pciHandler.offer(ctx.getProtocolChain());    
> +        
> +        ctx.recycle();
>          contexts.offer(ctx);
>      }
  ;-)


Jeanfrancois Arcand wrote:

> Salut,
>
> Oleksiy Stashok wrote:
>> Hello,
>>
>> IMHO to use correctly
>> ProtocolChainInstanceHandler.offer(ProtocolChain) - we need to add one
>> more method to ProtocolChain API:
>> ProtocolChainInstanceHandler getInstanceHandler();
>>
>> as at time, when ProtocolChain could be released - we don't know from
>> which ProtocolChainInstanceHandler the ProtocolChain was polled from.
>> Once method above will be added to the ProtocolChain interface - we
>> can easily implement ProtocolChain releasing in Controller's
>> returnContext() method:
>>
>>    public void returnContext(Context ctx){
>> /        ProtocolChain protocolChain = ctx.getProtocolChain();
>>        protocolChain.getInstanceHandler().offer(protocolChain);
>> /        contexts.offer(ctx);
>>    }
>>
>> What do you think?
>
> An alternative is to change the Controller and delegate the
> ProtocolChain configuration to the Context (see patch). That way no API
> changes :-)
>
> What do you think?
>
> -- Jeanfrancois
>
>
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Oleksiy Stashok
In reply to this post by Jeanfrancois Arcand-2
Hi,

I agree with your propose! :)
Also think implementation could become easier if we will add
Context.getProtocolChainInstanceHandler() like this:

    public ProtocolChainInstanceHandler getProtocolChainInstanceHandler() {
        ProtocolChainInstanceHandler protocolChainInstanceHandler =
                selectorHandler.getProtocolChainInstanceHandler();
        return protocolChainInstanceHandler != null ?
            protocolChainInstanceHandler :
            controller.getProtocolChainInstanceHandler();
    }

This way we will have that logic just in one single place :)

Thanks.

WBR,
Alexey.

Jeanfrancois Arcand wrote:

> Salut,
>
> Oleksiy Stashok wrote:
>> Hello,
>>
>> IMHO to use correctly
>> ProtocolChainInstanceHandler.offer(ProtocolChain) - we need to add
>> one more method to ProtocolChain API:
>> ProtocolChainInstanceHandler getInstanceHandler();
>>
>> as at time, when ProtocolChain could be released - we don't know from
>> which ProtocolChainInstanceHandler the ProtocolChain was polled from.
>> Once method above will be added to the ProtocolChain interface - we
>> can easily implement ProtocolChain releasing in Controller's
>> returnContext() method:
>>
>>    public void returnContext(Context ctx){
>> /        ProtocolChain protocolChain = ctx.getProtocolChain();
>>        protocolChain.getInstanceHandler().offer(protocolChain);
>> /        contexts.offer(ctx);
>>    }
>>
>> What do you think?
>
> An alternative is to change the Controller and delegate the
> ProtocolChain configuration to the Context (see patch). That way no
> API changes :-)
>
> What do you think?
>
> -- Jeanfrancois
>
>
>
> ------------------------------------------------------------------------
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: DefaultProtocolChainInstanceHandler

Jeanfrancois Arcand-2
Salut,

Oleksiy Stashok wrote:

> Hi,
>
> I agree with your propose! :)
> Also think implementation could become easier if we will add
> Context.getProtocolChainInstanceHandler() like this:
>
>    public ProtocolChainInstanceHandler getProtocolChainInstanceHandler() {
>        ProtocolChainInstanceHandler protocolChainInstanceHandler =
>                selectorHandler.getProtocolChainInstanceHandler();
>        return protocolChainInstanceHandler != null ?
>            protocolChainInstanceHandler :
>            controller.getProtocolChainInstanceHandler();
>    }
>
> This way we will have that logic just in one single place :)

+1. I will go ahead and apply :-)

A+

-- Jeanfrancois


>
> Thanks.
>
> WBR,
> Alexey.
>
> Jeanfrancois Arcand wrote:
>> Salut,
>>
>> Oleksiy Stashok wrote:
>>> Hello,
>>>
>>> IMHO to use correctly
>>> ProtocolChainInstanceHandler.offer(ProtocolChain) - we need to add
>>> one more method to ProtocolChain API:
>>> ProtocolChainInstanceHandler getInstanceHandler();
>>>
>>> as at time, when ProtocolChain could be released - we don't know from
>>> which ProtocolChainInstanceHandler the ProtocolChain was polled from.
>>> Once method above will be added to the ProtocolChain interface - we
>>> can easily implement ProtocolChain releasing in Controller's
>>> returnContext() method:
>>>
>>>    public void returnContext(Context ctx){
>>> /        ProtocolChain protocolChain = ctx.getProtocolChain();
>>>        protocolChain.getInstanceHandler().offer(protocolChain);
>>> /        contexts.offer(ctx);
>>>    }
>>>
>>> What do you think?
>>
>> An alternative is to change the Controller and delegate the
>> ProtocolChain configuration to the Context (see patch). That way no
>> API changes :-)
>>
>> What do you think?
>>
>> -- Jeanfrancois
>>
>>
>>
>> ------------------------------------------------------------------------
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]