Re: [PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

[Rob Shearman]

2008/10/21 Huw Davies :
> @@ -1728,6 +1767,10 @@ static HRESULT WINAPI DefaultHandler_IPersistStorage_HandsOffStorage(
>      if(SUCCEEDED(hr) && object_is_running(This))
>          hr = IPersistStorage_HandsOffStorage(This->pPSDelegate);
> - old_stg = InterlockedExchangePointer((void**)&This->storage, NULL);

IIRC, the default handler is apartment-threaded only, meaning that a
default handler object cannot be used on a thread other than the one
it was created on. Adding constructs like this introduce confusion to
future developers about whether the code is or isn't thread-safe and
so it should be avoided and a non-thread-safe construct used instead.
> - if(old_stg) IStorage_Release(old_stg);
> - This->storage_state = storage_state_uninitialised;
> - return hr;
>
> }

[PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

[Huw Davies]

On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
> 2008/10/21 Huw Davies :
> > @@ -1728,6 +1767,10 @@ static HRESULT WINAPI DefaultHandler_IPersistStorage_HandsOffStorage(
> >      if(SUCCEEDED(hr) && object_is_running(This))
> >          hr = IPersistStorage_HandsOffStorage(This->pPSDelegate);
> > - old_stg = InterlockedExchangePointer((void**)&This->storage, NULL);
>
> IIRC, the default handler is apartment-threaded only, meaning that a
> default handler object cannot be used on a thread other than the one
> it was created on.

That's true I believe.
> Adding constructs like this introduce confusion to
> future developers about whether the code is or isn't thread-safe and
> so it should be avoided and a non-thread-safe construct used instead.

I don't buy this.  Besides we're already using
Interlocked{In,De}crement().

Huw.

[PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

[Rob Shearman]

2008/10/22 Huw Davies :
> On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
> > 2008/10/21 Huw Davies :
> > > @@ -1728,6 +1767,10 @@ static HRESULT WINAPI DefaultHandler_IPersistStorage_HandsOffStorage(
> > >      if(SUCCEEDED(hr) && object_is_running(This))
> > >          hr = IPersistStorage_HandsOffStorage(This->pPSDelegate);
> > > - old_stg = InterlockedExchangePointer((void**)&This->storage, NULL);
> >
> > IIRC, the default handler is apartment-threaded only, meaning that a
> > default handler object cannot be used on a thread other than the one
> > it was created on.
>
> That's true I believe.
> > Adding constructs like this introduce confusion to
> > future developers about whether the code is or isn't thread-safe and
> > so it should be avoided and a non-thread-safe construct used instead.
>
> I don't buy this.  Besides we're already using
> Interlocked{In,De}crement().

The AddRef and Release code that uses Interlocked{In,De}crement() is
cookie cutter code that is pretty much the same in Wine for all COM
objects and so it doesn't look out of place. This does.

[PATCH 3/3] ole32: Call the appropriate storage function when we run the object.

[Huw Davies]

On Wed, Oct 22, 2008 at 02:03:26PM +0100, Rob Shearman wrote:
> 2008/10/22 Huw Davies :
> > On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
> > > 2008/10/21 Huw Davies :
> > > > @@ -1728,6 +1767,10 @@ static HRESULT WINAPI DefaultHandler_IPersistStorage_HandsOffStorage(
> > > >      if(SUCCEEDED(hr) && object_is_running(This))
> > > >          hr = IPersistStorage_HandsOffStorage(This->pPSDelegate);
> > > > - old_stg = InterlockedExchangePointer((void**)&This->storage, NULL);
> > >
> > > IIRC, the default handler is apartment-threaded only, meaning that a
> > > default handler object cannot be used on a thread other than the one
> > > it was created on.
> >
> > That's true I believe.
> > > Adding constructs like this introduce confusion to
> > > future developers about whether the code is or isn't thread-safe and
> > > so it should be avoided and a non-thread-safe construct used instead.
> >
> > I don't buy this.  Besides we're already using
> > Interlocked{In,De}crement().
>
> The AddRef and Release code that uses Interlocked{In,De}crement() is
> cookie cutter code that is pretty much the same in Wine for all COM
> objects and so it doesn't look out of place. This does.

I'm still not convinced, but there are more interesting things to have
debates about than this ;-)

Huw.