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

[Atsushi Eno]

Hi, sorry for the late reply, I'm back here again.

Thanks, I'm going to apply your patch with a little change. This method 
should not return true if the channel has received null HTTP context, 
while WaitOne() just returns true unless it goes timeout.

As for properties patch, I already made a relevant fix (I think I 
mentioned it in my previous posts) which fixed the issue you fixed in 
your own patch, and I didn't find it worthy enough to make an extra change.

Atsushi Eno


On 2010/04/08 18:35, Matt Dargavel wrote:
> Hi again,
>
> 	I'm just going through my outstanding local changes against the
> latest CVS.  Did you have chance to revisit the AutoResetEvent issue as
> mentioned below?  I've attached a new patch against the latest revision
> to show the changes.
>
> 	Also, I don't think the properties patch (my version or your
> version) got applied in the end?  I saw it got rolled back due to a
> regression.  If you could point me in the right direction I'd be happy
> to look in to this in a bit more.
>
> 		Regards,
>
> 			Matt.
>
>> -----Original Message-----
>> From: Matt Dargavel
>> Sent: 24 March 2010 12:29 PM
>> To: 'Atsushi Eno'
>> Cc: mono-devel-list at lists.ximian.com
>> Subject: RE: [PATCH] WCF multithreaded and property handling
>>
>> The problem I was trying to fix was that it's possible for wait to be
> set
>> to null after:
>>
>> if (wait != null)
>>
>> and before:
>>
>> wait.WaitOne(...)
>>
>> causing a null reference exception.
>>
>> Looking at MSDN it sounds like an AutoResetEvent should remain
> signalled
>> until a thread calls WaitOne?  The problem is if another thread sets
> the
>> event when it is already set.  If this happens the second Set has no
>> effect.  I don't think that's an issue here as the only place that
> sets the
>> event is the result of the operation we're starting?
>>
>> You're right that the waiting.Count>  0 check should have stayed in
> there.
>>
>> My thanks to you for all the work you've put in to WCF- in case you're
>> interested in how it's being used we're embedding a WCF web service in
> to
>> one of our core products (a SIP Switch) and then providing a set of
> web
>> pages that allow users to manage it.
>>
>> 	Matt.
>>
>>
>>> -----Original Message-----
>>> From: Atsushi Eno [mailto:atsushieno at veritas-vos-liberabit.com]
>>> Sent: 24 March 2010 10:58 AM
>>> To: Matt Dargavel
>>> Cc: mono-devel-list at lists.ximian.com
>>> Subject: Re: [PATCH] WCF multithreaded and property handling
>>>
>>> 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.
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>