Bug 71297 - dbus-daemon can only handle 64 simultaneous connections on Windows
Summary: dbus-daemon can only handle 64 simultaneous connections on Windows
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other Windows (All)
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-11-06 11:18 UTC by Cristian Oneț
Modified: 2015-01-05 16:00 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (1.78 KB, patch)
2014-01-27 11:29 UTC, Ralf Habacker
Details | Splinter Review
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (update 1) (2.02 KB, patch)
2014-01-27 11:35 UTC, Ralf Habacker
Details | Splinter Review
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (update 2) (1.85 KB, patch)
2014-01-27 12:03 UTC, Ralf Habacker
Details | Splinter Review

Description Cristian Oneț 2013-11-06 11:18:21 UTC
This is caused by the fact that select is used on the set of open file descriptors but, by default, the maximum number of file descriptors which can be handled by select is 64. See http://support.microsoft.com/kb/111855 for more details.

A fix for this is to add:

cmake\CMakeLists.txt:
SET(FD_SETSIZE "8192" CACHE STRING "The maximum number of connections that can be handled at once")

cmake\config.h.cmake
#define FD_SETSIZE @FD_SETSIZE@

This way the maximum number of simultaneous connections that can be handled on Windows can be set at compile time.

The longer fix would be to use WSAPoll instead of select (like poll on UNIX without limitations) but it needs Vista or higher so I guess that is a big drawback.
Comment 1 Simon McVittie 2013-11-06 11:45:20 UTC
(In reply to comment #0)
> A fix for this is to add:
> 
> cmake\CMakeLists.txt:
> SET(FD_SETSIZE "8192" CACHE STRING "The maximum number of connections that
> can be handled at once")
> 
> cmake\config.h.cmake
> #define FD_SETSIZE @FD_SETSIZE@
> 
> This way the maximum number of simultaneous connections that can be handled
> on Windows can be set at compile time.

I was about to object "that's not how C ABIs work..." but it seems that, on Windows, that does actually work, and is even documented to work. The parallel Autotools build system would also need this, something like this (pseudo-patch):

 if test "$dbus_win" = yes; then
     AC_DEFINE(DBUS_WIN,1,[...])
+    # Yes, on Windows it really does work like this.
+    # http://support.microsoft.com/kb/111855
+    AC_DEFINE([FD_SETSIZE], [8192],
+      [The maximum number of connections that can be handled at once])
     BUILD_TIMESTAMP=...

Ralf, does that seem sensible?

> The longer fix would be to use WSAPoll instead of select (like poll on UNIX
> without limitations) but it needs Vista or higher so I guess that is a big
> drawback.

I believe Ralf's stated policy is that after Microsoft stop offering security support for XP (early 2014), he'll stop supporting XP in D-Bus, at which point we could presumably increase our target version for _WIN32_WINNT and switch to WSAPoll.
Comment 2 LRN 2013-11-07 10:44:44 UTC
Increasing FD_SETSIZE is a valid way to make winsock2 select() work on more than 64 sockets.
Do note that this increases the size of fd_set variables accordingly (as they contain a FD_SETSIZE-long array of socket handles, not a bitfield).
Comment 3 Ralf Habacker 2014-01-27 10:49:59 UTC
(In reply to comment #1)
> (In reply to comment #0)
> > A fix for this is to add:
> > 
> > cmake\CMakeLists.txt:
> > SET(FD_SETSIZE "8192" CACHE STRING "The maximum number of connections that
> > can be handled at once")
> > 
> > cmake\config.h.cmake
> > #define FD_SETSIZE @FD_SETSIZE@
> > 
> > This way the maximum number of simultaneous connections that can be handled
> > on Windows can be set at compile time.
> 
> I was about to object "that's not how C ABIs work..." but it seems that, on
> Windows, that does actually work, and is even documented to work. The
> parallel Autotools build system would also need this, something like this
> (pseudo-patch):
> 
>  if test "$dbus_win" = yes; then
>      AC_DEFINE(DBUS_WIN,1,[...])
> +    # Yes, on Windows it really does work like this.
> +    # http://support.microsoft.com/kb/111855
> +    AC_DEFINE([FD_SETSIZE], [8192],
> +      [The maximum number of connections that can be handled at once])
>      BUILD_TIMESTAMP=...
> 
> Ralf, does that seem sensible?

looks good. 

BTW: Did not get notice of this bug because I'm not in the cc list. :-(

> > The longer fix would be to use WSAPoll instead of select (like poll on UNIX
> > without limitations) but it needs Vista or higher so I guess that is a big
> > drawback.
> 
> I believe Ralf's stated policy is that after Microsoft stop offering
> security support for XP (early 2014), he'll stop supporting XP in D-Bus,
> at which point we could presumably increase our target version for > _WIN32_WINNT and switch to WSAPoll.

which will be at april, 8 2014.
Comment 4 Ralf Habacker 2014-01-27 10:52:50 UTC
(In reply to comment #2)
> Increasing FD_SETSIZE is a valid way to make winsock2 select() work on more
> than 64 sockets.
> Do note that this increases the size of fd_set variables accordingly (as
> they contain a FD_SETSIZE-long array of socket handles, not a bitfield).

http://msdn.microsoft.com/de-de/library/windows/desktop/ms740141%28v=vs.85%29.aspx mentioned to use FD_... macros to provide portability, which is given by recent dbus code, so no problem here.
Comment 5 Ralf Habacker 2014-01-27 10:54:06 UTC
(In reply to comment #3)
> > Ralf, does that seem sensible?
> 
> looks good. 

should this go into master only or also into 1.8 ?
Comment 6 Simon McVittie 2014-01-27 11:09:01 UTC
(In reply to comment #5)
> should this go into master only or also into 1.8 ?

If in doubt, the answer is "only master" :-) Only change things in the stable branch if they're fixing a bug (as opposed to adding a feature), "uncontroversial", and not a large/structural/intrusive change.

I think adjusting FD_SETSIZE on Windows would be OK for 1.8, though, if you show me a specific, tested patch before committing.

Rewriting in terms of WSAPoll() is too intrusive for 1.8, but would be great for master (after we stop caring about XP).

> > SET(FD_SETSIZE "8192" CACHE STRING "The maximum number of connections that
> > can be handled at once")

To be clear: it is *not* OK to do this on Unix operating systems (where FD_SETSIZE is an immutable fact, and part of the OS ABI), so please make sure this is conditional on compiling for Windows.
Comment 7 Ralf Habacker 2014-01-27 11:29:07 UTC
Created attachment 92845 [details] [review]
Fix of 'dbus-daemon can only handle 64 simultaneous connections'
Comment 8 Ralf Habacker 2014-01-27 11:31:36 UTC
(In reply to comment #7)
> Created attachment 92845 [details] [review] [review]
> Fix of 'dbus-daemon can only handle 64 simultaneous connections'

Did read your last comment after uploading the patch. I need still to limit the FD_SIZES config.h variable to windows.
Comment 9 Ralf Habacker 2014-01-27 11:35:56 UTC
Created attachment 92846 [details] [review]
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (update 1)
Comment 10 Simon McVittie 2014-01-27 11:51:46 UTC
Comment on attachment 92846 [details] [review]
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (update 1)

Review of attachment 92846 [details] [review]:
-----------------------------------------------------------------

::: cmake/config.h.cmake
@@ +196,4 @@
>  /* Define to 1 if you have struct cmsgred */
>  #cmakedefine    HAVE_CMSGCRED 1
>  
> +#cmakedefine FD_SETSIZE @FD_SETSIZE@

If we're not on Windows, we don't want to #undef this. I'd prefer it further down (so it comes after we define DBUS_WIN), and wrapped in #ifdef DBUS_WIN.
Comment 11 Ralf Habacker 2014-01-27 12:03:26 UTC
Created attachment 92848 [details] [review]
Fix of 'dbus-daemon can only handle 64 simultaneous connections' (update 2)

do not undef FD_SETSIZE
Comment 12 Simon McVittie 2014-01-27 16:09:50 UTC
(In reply to comment #10)
> > +#cmakedefine FD_SETSIZE @FD_SETSIZE@
> 
> If we're not on Windows, we don't want to #undef this.

Having looked at the documentation for #cmakedefine, it expands to either "#define FD_SETSIZE 8192" or "/* #undef FD_SETSIZE */" (*not* actually undefining FD_SETSIZE), just like the Autotools equivalent.

So, either attachment #92846 [details] [review] or attachment #92848 [details] [review] is fine; please apply whichever you prefer. Sorry about that.
Comment 13 Ralf Habacker 2014-01-27 18:34:20 UTC
committed to dbus-1.8 and master
Comment 14 Simon McVittie 2015-01-05 16:00:58 UTC
This missed the boat for 1.8.14, because I prepared that release in advance (it was a security-hardening release under embargo). It'll be in 1.8.16 and 1.9.8.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.