[Mono-dev] [PATCH] WCF multithreaded and property handling

[Atsushi Eno]

After examining the patch, I have applied some parts of your patch.

-                wait = new AutoResetEvent (false);
-                source.ListenerManager.GetHttpContextAsync (timeout, 
HttpContextAcquired);
-                if (wait != null) // in case callback is done before 
WaitOne() here.
-                    return wait.WaitOne (timeout, false);
-                return waiting.Count > 0;
+                    var wait_ = new AutoResetEvent (false);
+                    wait = wait_;    // wait can be set to null if 
HttpContextAcquired runs to completion before we do WaitOne
+                    source.ListenerManager.GetHttpContextAsync 
(timeout, HttpContextAcquired);
+                    var result = wait_.WaitOne (timeout, false);
+                    return result;

This change is wrong. Since it is AutoResetEvent, it must not call 
WaitOne() if it has already finished (SignalAsyncWait). And It it set to 
null when SignalAsyncWait() is done. Also, it should not depend on 
specific GetHttpContextAsync() call result, as another async call may 
receive a context (hence waiting.Count > 0).

I think I have answered to all non-committed parts of your patch, but 
further comments are welcome. Thanks again for the patch. You're hero, 
few people dig in such depth into the WCF
core engine :)

Atsushi Eno


On 2010/03/23 22:33, Matt Dargavel wrote:
> Ok, no problem.  I can break them down more.
>
> You're right, I can provide no guarantees about the Thread.Sleep
> removal!  But it could have been related to locking registered_channels
> instead of pending?  I did quite a bit of multithreaded testing, and
> there were no problems; but I take your point.
>
> I implemented new functions rather than overriding Properties for a few
> reasons.  I wanted to be sure that I found everywhere that used the
> properties mechanism to check there were no unwanted side effects, and
> to make it more obvious something a little special was going on.  Also I
> thought that using a function hides the implementation from other
> classes.  For example, the .NET documentation states that
> ChannelListenerBase should search for the property and then delegate
> down the stack if it can't find it, so I could see a scenario where only
> certain properties were passed to / from inner channels.  But I guess
> that's refactoring and personal preference rather than a minimum change
> fix. :-)
>
> I can delve in to the test code and come up with some test cases for the
> properties fix, but unfortunately I think it'll be impossible to write
> test cases for the multithreading issues.
>
> 	Cheers,
>
> 		Matt.
>
>
>    
>> -----Original Message-----
>> From: Atsushi Eno [mailto:atsushieno at veritas-vos-liberabit.com]
>> Sent: 23 March 2010 12:50 PM
>> To: Matt Dargavel
>> Cc: mono-devel-list at lists.ximian.com
>> Subject: Re: [PATCH] WCF multithreaded and property handling
>>
>> Hello,
>>
>> Thanks for the patch. They are looking like a great set of attempts
>>      
> for
>    
>> cool bugfixes :) However there is a lot of other changes that your
>> description cannot explain. So, please first split the changes into
>>      
> further
>    
>> smaller patches for each purpose, and give explanation for each
>>      
> change. For
>    
>> example,
>>
>> - // FIXME: this should not be required, but it somehow saves some
>>      
> failures
>    
>> wrt concurrent calls.
>> - Thread.Sleep (100);
>>
>> This kind of change should not be made without explanation. (you
>>      
> aren't
>    
>> really sure about why it exists, right?)
>>
>> As for ChannelListenerBase.Properties, I'd rather make the changes
>>      
> much
>    
>> smaller like the attached change. Isn't it enough? There's no test
>>      
> case
>    
>> that I can verify your (and-my) changes.
>>
>> Atsushi Eno
>>
>> On 2010/03/23 20:28, Matt Dargavel wrote:
>>      
>>> The included patches fix the following:
>>>
>>> multithreaded_fixes.patch: ObjectDisposedException,
>>> InvalidOperationException("Another async TryReceiveRequest operation
>>> is in progress") and other multithreaded timing fixes. Also includes
>>> change to make GET ?wsdl case insensitive.
>>>
>>> properties_handling.patch: MetadataPublishingInfo not available in
>>> TransactionChannelListener's inner_listener. I created a new
>>> RetrieveProperty function as overriding GetProperty<T>  didn't work-
>>> the ChannelListenerBase implementation was still called. Perhaps
>>> there's a bug with generic function overrides or maybe I've done
>>> something silly there?
>>>
>>> properties_and_wsdl.patch: patch for ServiceMetadataExtension.cs
>>>        
> that
>    
>>> goes with the properties changes and the ?wsdl change.
>>>
>>> Let me know if you have any questions. :-)
>>>
>>> Matt.
>>>
>>>        
>
>
>