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. > > >>> > > >>> > > > > > > > > >
HttpReplyChannel.patch
Description: HttpReplyChannel.patch
_______________________________________________ Mono-devel-list mailing list Mono-devel-list@lists.ximian.com http://lists.ximian.com/mailman/listinfo/mono-devel-list