Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#17937 closed defect (fixed)

wxFTP in worker thread truncates files (wxSocketBase race condition)

Reported by: aebailey82 Owned by: Vadim Zeitlin <vadim@…>
Priority: normal Milestone: 3.1.1
Component: network Version: 3.1.0
Keywords: wxFTP wxHTTP wxSocketBase thread Cc:
Blocked By: Blocking:
Patch: yes

Description

I am trying to download a file with wxFTP in a worker thread. Sometimes I get zero bytes, or otherwise less than the whole file. Here's what I think is happening:

  • Server closes the connection to indicate end of file.
  • In the main thread, wxSocket_Internal_WinProc gets FD_CLOSE and wxSocketBase::OnRequest() clears m_connected.
  • Worker thread fails in wxSocketBase::DoRead() because m_connected == false.

The attached program demonstrates the problem using wxFTP. wxHTTP behaves the same way. I am using wxWidgets 3.1.0 with MSVC 2015 on Windows 10. Also tried latest code from GitHub.

It seems to me that wxSocketBase::m_connected does not accurately indicate when data is available on the socket, and therefore we should not check it before calling m_impl->Read(). This change does fix my problem. However, I hesitate to suggest this patch because I see the code was that way before but changed to fix something else.

Another option might be to prevent the GUI thread from clearing m_connected somehow. I just want a blocking socket, no events.

#17031 looks very similar, but I have the fix and still this problem. Also wxFTP::GetPassivePort() bypasses wxProtocol and creates a wxSocketClient instead, so that logic in wxProtocol() to set wxSOCKET_BLOCK doesn't happen.

If this is a real problem I can't believe I'm the only one who's seen it. Am I missing something?

Attachments (2)

sock_race_ftp.cpp download (2.1 KB) - added by aebailey82 3 years ago.
problem demonstration
no check m_connected.patch download (763 bytes) - added by aebailey82 3 years ago.
patch for discussion, not recommended

Download all attachments as: .zip

Change History (11)

Changed 3 years ago by aebailey82

problem demonstration

Changed 3 years ago by aebailey82

patch for discussion, not recommended

comment:1 Changed 3 years ago by vadz

  • Status changed from new to confirmed

I think we need to do the equivalent of this commit for MSW as it makes absolutely no sense to use events for sockets in thread other than main and it is guaranteed to result in data races, at the very least.

Also, thanks for pointing out the problem in GetPassivePort() -- this should be simple enough to fix and I'll do it. BTW, it's not clear if you have this problem only with passive FTP or not?

comment:2 Changed 3 years ago by vadz

  • Milestone set to 3.1.1
  • Patch set

So I've tried to fix this in this PR. Could you please check if it helps?

I'd also be very grateful for other people who use wxSocket in their code if they could please test these changes to check if they don't result in any regressions. TIA!

comment:3 Changed 3 years ago by aebailey82

  • Cc aebailey@… added

Your PR fixes wxHTTP and passive FTP.

Active FTP does exhibit the same problem, even after merging the PR. Adding the same socket flags to wxSocketServer() in wxFTP::GetActivePort() seems to fix it.

Many thanks for your speedy help, and for your continuing work on wxWidgets.

comment:4 Changed 3 years ago by vadz

Thanks for testing!

What about the latest PR version, which adds a fix for active FTP and corrects a too eager assert which broke the test suite?

comment:5 Changed 3 years ago by aebailey82

  • Cc aebailey@… removed

Works for me.

comment:6 Changed 3 years ago by Vadim Zeitlin <vadim@…>

In 40774e1cc/git-wxWidgets:

Use blocking sockets from non-main threads for passive FTP too

This extends the changes of d421373c2eee777fd1300fcfb56971f9248ba382
to the case of passive FTP, which created wxSocketClient directly and so
didn't use the correct flags when used from a worker thread.

See #17937.

comment:7 Changed 3 years ago by Vadim Zeitlin <vadim@…>

In 8d66bfd7e/git-wxWidgets:

Use blocking server socket in non-main threads for active FTP

This is similar to the previous commit, but for active FTP connections.
wxSocketServer was also created directly in this case, without using
wxProtocol ctor, so wxSOCKET_BLOCK must be explicitly specified when
creating it from worker thread, just as it was already done in
d421373c2eee777fd1300fcfb56971f9248ba382 for the other connections and
in the previous commit for passive FTP ones.

See #17937.

comment:8 Changed 3 years ago by Vadim Zeitlin <vadim@…>

  • Owner set to Vadim Zeitlin <vadim@…>
  • Resolution set to fixed
  • Status changed from confirmed to closed

In dadb7b9fb/git-wxWidgets:

Don't register async notifications for MSW blocking sockets

Under MSW calling UnblockAndRegisterWithEventLoop() for blocking sockets
is not only useless, but actually harmful when the socket is used from a
worker thread (which is the common case for blocking sockets), as it
means that the main thread will be receiving notifications for the
socket events and modifying the socket object while it's being used from
the other thread, resulting in data races and general brokenness.

This is similar to e18c8fd29a10b1b87ea5cff7c5b3a16fe5b32690 for Unix
sockets.

Closes #17937.

comment:9 Changed 3 years ago by Vadim Zeitlin <vadim@…>

In b8ff7114/git-wxWidgets:

Merge branch 'blocking-sockets-fixes'

Closes #17937.

Note: See TracTickets for help on using tickets.