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

[Matt Dargavel]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: HttpReplyChannel.patch
Type: application/octet-stream
Size: 959 bytes
Desc: HttpReplyChannel.patch
Url : http://lists.ximian.com/pipermail/mono-devel-list/attachments/20100408/7bca6f9d/attachment-0001.obj