wxSocketBase.LastCount() and thread-safety

[mar]

Assume the following scenario:
A blocking socket keeps reading in a background thread,
while it's possible to send from another thread on the same socket.
The only way (AFAIK) to get received count is by calling LastCount()
on a socket. The problem is that m_lcount is also set by Write.
It can happen that another thread modifies m_lcount before the read thread
calls LastCount().
I encountered crashes when using wxDataGramSocket in such scenario.
The workround is to use a nonblocking socket,
sleep in the read thread and encapsulate all SendTo/RecvFrom calls with a mutex,
which is certainly a bad solution.
I believe this is a design problem and Recv/Read should return read count instead of ref to socket. The same for Send/Write.

[vadz]

We probably need to put critical section around m_lcount access.

Although I admit I don't quite understand why would you want to write to the same socket you're blocking on in another thread.

As always, it would be very useful to have the smallest possible example reproducing the issue.

[mar]

Well m_lcount access itself won't solve anything. The design of LastCount() is simply bad
and no mutex anywhere will solve that.
Returning read count / write count could do. But it would also break compatibility.

Why I need that: when using UDP a socket can't bind twice to same local port.
So by using two sockets (one for writing and one for reading),
I would waste two local ports instead of one just because of poor design of wxSocketBase.
Of course I can use async socket which I am doing right now.

Note that this (writing to a socket which blocks on read in another thread) should work fine when using system native sockets directly.

Nevermind, thanks. And sorry for posting no example. I guess I better close this as wontfix.

[rowbearto]

I was wondering if this could be reopened?

I see the same potential issue here on regular TCP/IP sockets. I may create a small sample to illustrate the issue. I agree with the original poster that just protecting m_lcount with a critical section is not sufficient. My application is full duplex, both Read()/LastCount() and Write()/LastCount() can be occurring independently in different threads, and it is possible for one to interrupt the other between the call to [Write()/Read()] and the subsequent LastCount().

I think there is a possible solution to this that also retains backwards compatibility. I propose to add new member functions LastCountRx() and LastCountTx(), as well as to keep the existing LastCount() function to not break backwards compatibility.

Then, inside of wxSocketBase, we manage 2 new member variables: m_lcount_rx and m_lcount_tx. We still retain the old m_lcount.

Inside of the function call to wxSocketBase::Read(), we replace the existing code of:

m_lcount = DoRead(buffer, nbytes);

with this:

m_lcount_rx = DoRead(buffer, nbytes);
m_lcount = m_lcount_rx;

We do a similar thing for Write() and m_lcount_tx.

Then the LastCountRx() function will return m_lcount_rx, while the LastCountTx() function will return m_lcount_tx. The legacy LastCount() function will still return m_lCount for backwards compatibility.

I think the same could be done for UDP sockets but I did not look into that code.

Additionally, in my application, I would like to block on Write() (running in worker thread), but not block on Read() (running in Main thread). With the current API, this is not possible as one flag controls both. So I would also like to add 4 new flags that can be passed to SetFlags(): wxSOCKET_WAITALL_RX, wxSOCKET_NOWAIT_RX, wxSOCKET_WAITALL_TX, and wxSOCKET_NOWAIT_TX. These will have the same meaning as the original flags, but only for the one direction at a time.

With the 4 new flags, we can continue to manage in the legacy way, or one can choose to use the Thread safe way.

If I made this change myself and wanted to submit it, how to do that? Is there a page somewhere that describes how to do this.

[vadz]

This could indeed work although I'm far from certain that doing this will really fix all the MT-safety problems in wxSocket.

I'd prefer the new functions to be called LastReadCount() and LastWrittenCount() which IMHO would be more clear than Rx and Tx which we don't use anywhere else.

Please read our HotToSubmitPatches to learn about how to submit your changes. If you have any questions, please post them to wx-dev mailing list (it's a better place for any relatively long discussions than Trac). Also, please submit a patch for this and a separate patch for the flags change if possible.

TIA!

[rowbearto]

Patch adding GetReadCount(), GetWriteCount(), wxSOCKET_NOWAIT_READ, wxSOCKET_NOWAIT_WRITE, wxSOCKET_WAITALL_READ, wxSOCKET_WAITALL_WRITE

[rowbearto]

I created a patch that implemented all of the changes from my last post.

I used your suggestion for naming and used Read/Write instead of Rx/Tx.

Additionally, I added some notes on how to build/run the unit tests on VS2008, hope you don't mind. This is the new file tests/install_vc9.txt

The changes I have made do not break any of the unit tests, and I added some of the new functionality to the unit tests.

I added documentation for all of these updates, and I also added some documentation for how to utilize SetFlags() to ensure thread-safety when using ReadMsg() and WriteMsg().

I also tested these changes on some code I have that has heavy socket traffic, it seems to have no issue.

[vadz]

Thanks for the patch, I'm going to apply it soon.

I don't know what to do about your test install notes. On one hand, they're definitely valuable, OTOH we already have source:wxWidgets/trunk/docs/tech/tn0017.txt which it partially duplicates. I think it would make sense to merge it with that tech note and maybe also add a README to the tests directory pointing to that note. Or move the tech note (which, it is true, nobody seems to be able to find on their own) to the tests directory? If you can do this, could you please open another ticket for this?

Finally, it might also be useful to put this information on a wiki page so that others could complete/extend it.

[VZ]

(In [72591]) Add per-direction wxSocket wait flags and byte counters.

Allow to specify whether the socket should block until all the data is read or
written or, on the contrary, avoid blocking only when reading or writing
instead of always using the same behaviour in both directions.

Also add separate counters for the bytes read/written instead of using the
same one for both.

These changes make it possible to use the same socket for reading/writing in
different threads.

Closes #14506.

[VZ]

(In [72828]) Define wxSOCKET_XXX flags as wxSOCKET_XXX_READ|wxSOCKET_XXX_WRITE.

The recently introduced (in r72591) wxSOCKET_{WAITALL,NOWAIT}_{READ,WRITE}
flags weere for some reason completely different and unrelated to the existing
bidirectional wxSOCKET_{WAITALL,NOWAIT} ones. Change this by defining the
bidirectional version simply as the sum of the two others. This makes much
more sense than testing for either wxSOCKET_XXX or wxSOCKET_XXX_READ or
wxSOCKET_XXX_WRITE being specified.

And it also fixes an assert in wxSocketWaitModeChanger where a sanity check
failed when this class was used with wxSOCKET_WAITALL|wxSOCKET_WAITALL_READ.

See #14506.