[Mono-dev] [PATCH] WCF multithreaded and property handling
matt at shout-telecoms.com
Wed Mar 24 08:29:01 EDT 2010
The problem I was trying to fix was that it's possible for wait to be
set to null after:
if (wait != null)
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
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.
> -----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,
> - 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
> 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
> > 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
> > reasons. I wanted to be sure that I found everywhere that used the
> > properties mechanism to check there were no unwanted side effects,
> > to make it more obvious something a little special was going on.
> > 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
> > certain properties were passed to / from inner channels. But I
> > that's refactoring and personal preference rather than a minimum
> > fix. :-)
> > I can delve in to the test code and come up with some test cases for
> > properties fix, but unfortunately I think it'll be impossible to
> > 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
> >>> is in progress") and other multithreaded timing fixes. Also
> >>> 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
> >>> 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.
More information about the Mono-devel-list