Re: [PATCH 3/3] ole32: Call the appropriate storage function when we run the object.
2008/10/21 Huw Davies <huw(a)codeweavers.com>:
@@ -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; }
-- Rob Shearman
On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
2008/10/21 Huw Davies <huw(a)codeweavers.com>:
@@ -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. -- Huw Davies huw(a)codeweavers.com
2008/10/22 Huw Davies <huw(a)codeweavers.com>:
On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
2008/10/21 Huw Davies <huw(a)codeweavers.com>:
@@ -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. -- Rob Shearman
On Wed, Oct 22, 2008 at 02:03:26PM +0100, Rob Shearman wrote:
2008/10/22 Huw Davies <huw(a)codeweavers.com>:
On Wed, Oct 22, 2008 at 12:46:39PM +0100, Rob Shearman wrote:
2008/10/21 Huw Davies <huw(a)codeweavers.com>:
@@ -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. -- Huw Davies huw(a)codeweavers.com
participants (2)
-
Huw Davies -
Rob Shearman