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@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:atsushi...@veritas-vos-liberabit.com] >>> Sent: 24 March 2010 10:58 AM >>> To: Matt Dargavel >>> Cc: mono-devel-list@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:atsushi...@veritas-vos-liberabit.com] >>>>> Sent: 23 March 2010 12:50 PM >>>>> To: Matt Dargavel >>>>> Cc: mono-devel-list@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. >>>>>> >>>>>> >>>> >>>> >>>> > _______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list