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

[Matt Dargavel]

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.
> >>>
> >>>
> >
> >
> >