wxGTK: wxSocketBase is not thread safe

[Chris Stankevitz]

This email is a follow up of http://trac.wxwidgets.org/ticket/12886

First let me say that the patch I created in ticket 12886 was not
intended to be committed. It is simply what I am using to work around
a problem (which could be of my own making). I tried to highlight
this with the word "hack" in the patch filename. My rambling comments
on the bug report (as I was learning) just served to make the bug
report a big mess. I apologize for that.

Second let me say that I do not understand wxSocket code very well and
I could be way off base so please correct if/when appropriate.

Third let me say that I know wxWidgets does not advertise itself to be
thread safe (except a couple wxEvtHandler functions).

For the rest of this email I will use these terms:

GTK Events: GTK callbacks that the wxSocket code uses

wxSocket Events: wxSocketEvent callbacks that wxWidgets users receive
to be notified of changes to the socket state

Main Thread: the main wxWidgets thread

Worker Thread: a worker thread created by an application

===

Problem:
A wxGTK application will crash if it attempts to use a wxSocket in a
Worker Thread

Reason:
wxSockets use GTK Events (which I do not understand). These GTK
Events occur on the Main Thread and use the socket. When the Worker
Thread attempts to use the socket concurrently, a crash can occur.

My use case:
I use wxSockets in the following mode:
1. With wxSOCKET_NOWAIT
2. Without wxSocket Events (i.e. wxSocket::SetNofify = false)

When I want to write data, I call wxSocket::Write. If the data does
not get written immediately, I will come back later and call Write
again.

When I want to read data, I call wxSocket::Read. If there is no data
to read, I will try again later.

Note I am not instigated to read by a wxSocket Event, I just do it on
my own.

Bold proclamation:
Although I do not understand what GTK Events do, when (1) and (2) are
true, GTK Events are not needed. I "proved" this to myself with the
patch in bug 12886.

Conclusion:
3. wxSocket uses wxGTK Events which makes wxSocket not thread safe.
4. GTK Events are not needed when (1) and (2)
5. For the special case of (1) and (2), GTK Events can be disabled,
making wxSocket thread safe

My patch in bug report 12886 implements (5).

[Vadim Zeitlin]

On Tue, 1 May 2012 10:42:52 -0700 (PDT) Chris Stankevitz wrote:

CS> This email is a follow up of http://trac.wxwidgets.org/ticket/12886

Hi Chris,

Thanks for following up.

CS> First let me say that the patch I created in ticket 12886 was not
CS> intended to be committed. It is simply what I am using to work around
CS> a problem (which could be of my own making). I tried to highlight
CS> this with the word "hack" in the patch filename. My rambling comments
CS> on the bug report (as I was learning) just served to make the bug
CS> report a big mess. I apologize for that.

No need to apologize and thanks for looking at this at all, I do
appreciate the amount of courage that diving into wxSocket code requires!

CS> For the rest of this email I will use these terms:
CS>
CS> GTK Events: GTK callbacks that the wxSocket code uses

Maybe we should call those "GTK callbacks" to avoid confusion with wx
events?


CS> Problem:
CS> A wxGTK application will crash if it attempts to use a wxSocket in a
CS> Worker Thread

The problem here is that wxSocket is, IMO/AFAIK, supposed to be used in
non-blocking mode from the main thread or in blocking mode (i.e. with
wxSOCKET_BLOCK flag) from a worker one. Apparently nobody, and definitely
not me, thought about using wxSOCKET_NOWAIT from a worker thread...

CS> Reason:
CS> wxSockets use GTK Events (which I do not understand).

GTK callbacks are used to integrate the sockets with the event loop.
They're necessary to allow using sockets asynchronously (i.e. in
non-blocking mode) in the main thread: instead of blocking on the socket
until it has something to read, or polling it, we just wait for something
to become available and are notified by this via the event loop. So this is
something that can only be done in the main thread.

CS> My use case:
CS> I use wxSockets in the following mode:
CS> 1. With wxSOCKET_NOWAIT
CS> 2. Without wxSocket Events (i.e. wxSocket::SetNofify = false)

The socket events are actually, I think, completely orthogonal to
everything else. I.e. they don't fundamentally modify the socket behaviour.

CS> When I want to write data, I call wxSocket::Write. If the data does
CS> not get written immediately, I will come back later and call Write
CS> again.
CS>
CS> When I want to read data, I call wxSocket::Read. If there is no data
CS> to read, I will try again later.
CS>
CS> Note I am not instigated to read by a wxSocket Event, I just do it on
CS> my own.

I see, thanks for the explanation. So this is actually a non-blocking-
but-polling mode which is something yet different from the 2 modes I had in
mind (fully asynchronous in the main thread or blocking in the worker).


CS> Bold proclamation:
CS> Although I do not understand what GTK Events do, when (1) and (2) are
CS> true, GTK Events are not needed. I "proved" this to myself with the
CS> patch in bug 12886.

Yes, I agree.

CS> Conclusion:
CS> 3. wxSocket uses wxGTK Events which makes wxSocket not thread safe.
CS> 4. GTK Events are not needed when (1) and (2)
CS> 5. For the special case of (1) and (2), GTK Events can be disabled,
CS> making wxSocket thread safe
CS>
CS> My patch in bug report 12886 implements (5).

I wonder if we shouldn't go further and just always avoid the use of GTK
callbacks in non-main thread? Can you (or anybody else brave enough to read
this thread) think of any case when they would be needed? This would be
simpler than trying to enable the events dynamically IMHO.

What do you think?
VZ

[Chris Stankevitz]

CS> I use wxSockets in the following mode:
CS> 1. With wxSOCKET_NOWAIT
CS> 2. Without wxSocket Events (i.e. wxSocket::SetNofify = false)

This went without saying, but to make reference below, I will add a
third condition:
0. On a worker thread

VZ>  The socket events are actually, I think, completely orthogonal to
VZ> everything else. I.e. they don't fundamentally modify the socket behaviour.

I believe your implication is that (2) has no bearing on the problem
or the solution, which I will address below.

VZ> I wonder if we shouldn't go further and just always avoid the use of GTK
VZ> callbacks in non-main thread?

Allow me to restate this as "Solution A":

Solution A:
When (0) is in effect, disable GTK callbacks. I'm not sure how you
would implement this. Perhaps:
- Sockets must be constructed only on the thread they will live on
- wxSocket constructor will use IsMain to decide if it is on the main
thread or not and enable/disable GTK callbacks accordingly

Solution B:
When (1) and (2) are in effect, disable GTK callbacks. This is what
my hack/patch does.

Solution C:
[other ideas here]


Potential problem with Solution A:

Is it possible issue wxSocketEvents when these are both true:
- GTK callbacks are disabled
- socket reading/writing happens on a worker thread?

I assumed the answer is no, which is why I made (2) a condition for Solution B.

Chris

[Vadim Zeitlin]

On Wed, 2 May 2012 10:00:29 -0700 Chris Stankevitz wrote:

CS> CS> I use wxSockets in the following mode:
CS> CS> 1. With wxSOCKET_NOWAIT
CS> CS> 2. Without wxSocket Events (i.e. wxSocket::SetNofify = false)
CS>
CS> This went without saying, but to make reference below, I will add a
CS> third condition:
CS> 0. On a worker thread

Actually this got me thinking: is this really so? I.e. do we need to use
GTK callbacks when (1) and (2) are true even when using the socket on the
main thread? From reading the code it looks like we don't need them because
the GTK callbacks just call wxSocketBase::OnRequest() which is used for 2
things only:

(a) Set/update m_eventsgot flags.
(b) Generate a wxSocketEvent.

We don't need (b) because of (2) and m_eventsgot is only used in DoWait()
which is not normally used with wxSOCKET_NOWAIT sockets, i.e. not called
from DoRead() and DoWrite() for them.

Although now I wonder if wxSocket::Wait() might still be called explicitly
by the user code even for a socket with wxSOCKET_NOWAIT flag. Does this
make sense? I'm not sure :-(

Anyhow, it looks like the following should work:

- When the flags are set to include wxSOCKET_NOWAIT, disable the callbacks
(and when wxSOCKET_NOWAIT is cleared, re-enable them).

- If DoWait() is still called, and it must be called explicitly for a
wxSOCKET_NOWAIT socket, then enable the callbacks on entry to it and
disable them before returning.

What do you think? If you don't see any obvious problems with this
approach, would you be able to try to make these changes and test them with
your application? TIA!


CS> Allow me to restate this as "Solution A":
CS>
CS> Solution A:
CS> When (0) is in effect, disable GTK callbacks. I'm not sure how you
CS> would implement this. Perhaps:
CS> - Sockets must be constructed only on the thread they will live on

No, this would break existing code creating a socket via Accept() on the
main thread and then passing it to another one. The original example from
http://trac.wxwidgets.org/ticket/12886 does exactly this BTW.

CS> - wxSocket constructor will use IsMain to decide if it is on the main
CS> thread or not and enable/disable GTK callbacks accordingly

I thought about testing for this in Do{Read,Write}() and not ctor but now
I'm not even sure if we need this, if the above works we could just always
keep the events disabled in wxSOCKET_NOWAIT case.

Now it only remains to test whether it actually works...

Thanks again for your help,
VZ

[Chris Stankevitz]

On Thu, May 3, 2012 at 6:10 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
VZ> the GTK callbacks ... used for
VZ> (b) Generate a wxSocketEvent.

This is what I suspected. I think this means that if we do disable
GTK callbacks, it must only be when the user has disabled
wxSocketEvents by calling wxSocket::Notify(false).

VZ> - When the flags are set to include wxSOCKET_NOWAIT, disable the callbacks
VZ>  (and when wxSOCKET_NOWAIT is cleared, re-enable them).

I think you need to add the additional condition "the user has
disabled wxSocketEvents". I'll assume you agree for the duration of
this message.

VZ> - If DoWait() is still called, and it must be called explicitly for a
VZ>  wxSOCKET_NOWAIT socket, then enable the callbacks on entry to it and
VZ>  disable them before returning.

Ah yes, I did not consider this.

VZ> What do you think?

You and I have come to the same conclusion. I will restate it as Solution C.

Solution C:
When
- wxSOCKET_NOWAIT and
- wxSocket::SetNofify = false
Then
- Disable GTK events
Except
- Temporarily enable GTK events within within OnWait

VZ> would you be able to try to make these changes and test them with
VZ> your application? TIA!

Except for enabling GTK callbacks during DoWait(), my patch
gtk-socket-thread-safe-hack.patch implements this. It works well on
linux and windows (effectively a noop on windows). I do not use
OnWait, but I can add it to the patch.

Chris

[Vadim Zeitlin]

On Thu, 3 May 2012 13:50:25 -0700 Chris Stankevitz wrote:

CS> On Thu, May 3, 2012 at 6:10 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
CS> VZ> the GTK callbacks ... used for
CS> VZ> (b) Generate a wxSocketEvent.
CS>
CS> This is what I suspected. I think this means that if we do disable
CS> GTK callbacks, it must only be when the user has disabled
CS> wxSocketEvents by calling wxSocket::Notify(false).
CS>
CS> VZ> - When the flags are set to include wxSOCKET_NOWAIT, disable the callbacks
CS> VZ>  (and when wxSOCKET_NOWAIT is cleared, re-enable them).
CS>
CS> I think you need to add the additional condition "the user has
CS> disabled wxSocketEvents". I'll assume you agree for the duration of
CS> this message.

Yes, right, thanks.

CS> VZ> - If DoWait() is still called, and it must be called explicitly for a
CS> VZ>  wxSOCKET_NOWAIT socket, then enable the callbacks on entry to it and
CS> VZ>  disable them before returning.
CS>
CS> Ah yes, I did not consider this.
CS>
CS> VZ> What do you think?
CS>
CS> You and I have come to the same conclusion. I will restate it as Solution C.
CS>
CS> Solution C:
CS> When
CS> - wxSOCKET_NOWAIT and
CS> - wxSocket::SetNofify = false
CS> Then
CS> - Disable GTK events
CS> Except
CS> - Temporarily enable GTK events within within OnWait
CS>
CS> VZ> would you be able to try to make these changes and test them with
CS> VZ> your application? TIA!
CS>
CS> Except for enabling GTK callbacks during DoWait(), my patch
CS> gtk-socket-thread-safe-hack.patch implements this.

OK, I see it now, thanks for all the explanations. I think the names of
the functions and variables used in the patch is a bit misleading: this is
not about being thread-safe but about needing callbacks at all. So I'd
rename m_threadsafe to m_needsEvents (this would also inverse its meaning,
of course) and MakeThreadSafe() to SetNeedsEvents(). What do you think?

CS> It works well on linux and windows (effectively a noop on windows).

Does it also fix the problem demonstrated by the original example in
http://trac.wxwidgets.org/ticket/12886 (main.cpp/test.py)?

CS> I do not use OnWait, but I can add it to the patch.

This would be perfect.

Thanks in advance,
VZ

[Chris Stankevitz]

On Fri, May 4, 2012 at 11:12 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
>  OK, I see it now, thanks for all the explanations. I think the names of
> the functions and variables used in the patch is a bit misleading: this is
> not about being thread-safe but about needing callbacks at all. So I'd
> rename m_threadsafe to m_needsEvents (this would also inverse its meaning,
> of course) and MakeThreadSafe() to SetNeedsEvents(). What do you think?

wxSockImpl::MakeThreadSafe() is a terrible name.

wxSockImpl::SetNeedsEvents() might be a strange name too given
1. 'Events' (which we are calling 'GTK callbacks' in this email
thread) might be confused with wxSocketEvents.
2. This function will exist at the wxSockImpl layer which is visible
to all builds, yet only the GTK build has "GTK callbacks". In the msw
build, for example, the function will do nothing and possibly be
confused for wxSocketEvents.

But I'll make the change.

>  Does it also fix the problem demonstrated by the original example in
> http://trac.wxwidgets.org/ticket/12886 (main.cpp/test.py)?

My fix will not help the original example because the original example
does not use wxSOCKET_NOWAIT.

Solution C:
When
- wxSOCKET_NOWAIT and

- wxSocket::SetNofify = false

Then
- Disable GTK events
Except

- Temporarily enable GTK events within OnWait

Chris

[Vadim Zeitlin]

On Sun, 6 May 2012 09:12:12 -0700 Chris Stankevitz wrote:

CS> On Fri, May 4, 2012 at 11:12 AM, Vadim Zeitlin <va...@wxwidgets.org> wrote:
CS> >  OK, I see it now, thanks for all the explanations. I think the names of
CS> > the functions and variables used in the patch is a bit misleading: this is
CS> > not about being thread-safe but about needing callbacks at all. So I'd
CS> > rename m_threadsafe to m_needsEvents (this would also inverse its meaning,
CS> > of course) and MakeThreadSafe() to SetNeedsEvents(). What do you think?
CS>
CS> wxSockImpl::MakeThreadSafe() is a terrible name.
CS>
CS> wxSockImpl::SetNeedsEvents() might be a strange name too given
CS> 1. 'Events' (which we are calling 'GTK callbacks' in this email
CS> thread) might be confused with wxSocketEvents.
CS> 2. This function will exist at the wxSockImpl layer which is visible
CS> to all builds, yet only the GTK build has "GTK callbacks". In the msw
CS> build, for example, the function will do nothing and possibly be
CS> confused for wxSocketEvents.
CS>
CS> But I'll make the change.

I'd welcome a better name, please feel free to use something better if you
have any ideas. I agree that SetNeedsEvents() is not great but OTOH we do
have ReenableEvents() already in wxSocketImpl so at least it's consistent.

CS> >  Does it also fix the problem demonstrated by the original example in
CS> > http://trac.wxwidgets.org/ticket/12886 (main.cpp/test.py)?
CS>
CS> My fix will not help the original example because the original example
CS> does not use wxSOCKET_NOWAIT.

Oops, sorry, I saw wxSOCKET_NOWAIT there but actually it's only used for
the listening socket, so yes, this is yet another problem/bug :-(

CS> Solution C:
CS> When
CS> - wxSOCKET_NOWAIT and
CS> - wxSocket::SetNofify = false
CS> Then
CS> - Disable GTK events
CS> Except

CS> - Temporarily enable GTK events within OnWait

Looks perfect to me, thanks again for looking at this!
VZ