New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase FD_SETSIZE #153

Closed
TankOs opened this Issue Jan 6, 2012 · 14 comments

Comments

Projects
None yet
5 participants
@TankOs
Member

TankOs commented Jan 6, 2012

On Windows, FD_SETSIZE (the maximum number of allowed entries in the fd_set structure) is limited to 64 which is quite low. So when doing a select() with more than 64 you run into trouble.

One could either create groups with FD_SETSIZE entries and call select() multiple times or increase FD_SETSIZE by defining the macro before Winsock2.h is included.

On Linux the default is 1024.

More info: http://msdn.microsoft.com/en-us/library/windows/desktop/ms740141(v=vs.85).aspx

@ghost ghost assigned LaurentGomila Jan 6, 2012

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 6, 2012

Member

Do you already have an idea? I'm just thinking about that too any ask myself if

  • calling select() multiple times or
  • calling select() once and iterating over all possible sockets

is faster.

Member

TankOs commented Jan 6, 2012

Do you already have an idea? I'm just thinking about that too any ask myself if

  • calling select() multiple times or
  • calling select() once and iterating over all possible sockets

is faster.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 6, 2012

Member

How can you call select() multiple times? The first call will block the others.

Member

LaurentGomila commented Jan 6, 2012

How can you call select() multiple times? The first call will block the others.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 6, 2012

Member

Okay, should think before posting. ;) However it still applies for a zero timeout and/or multiple threads, but that only complicates things.

So probably increasing FD_SETSIZE manually is a good choice then.

Member

TankOs commented Jan 6, 2012

Okay, should think before posting. ;) However it still applies for a zero timeout and/or multiple threads, but that only complicates things.

So probably increasing FD_SETSIZE manually is a good choice then.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 6, 2012

Member

Yep :)

Member

LaurentGomila commented Jan 6, 2012

Yep :)

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 8, 2012

Member

I investigated a little bit further: It seems that redefining FD_SETSIZE on Windows may have serious side-effects, it's really not recommended. Therefore we end up with a 64 sockets limit. There're actually two ways I encountered that solve the problem:<

  • Use multiple threads for multiple select()s and use a maximum of 64 sockets per thread for the fd_set. Not possible: The highest file descriptor ID counts, not the amount.
  • Use WSAPoll(), however that one has been added in Windows Vista and is therefore not available in XP, which might be a real showstopper (also it's Windows-only, on Linux you still have the 1024 limit (in detail it means the highest file descriptor ID must be less than 1024, not the amount!!))

Seems like wrapping the sockets API isn't enough. SFML Network probably needs another layer of abstraction if you want to keep it simple to use without annoying limitations.

Member

TankOs commented Jan 8, 2012

I investigated a little bit further: It seems that redefining FD_SETSIZE on Windows may have serious side-effects, it's really not recommended. Therefore we end up with a 64 sockets limit. There're actually two ways I encountered that solve the problem:<

  • Use multiple threads for multiple select()s and use a maximum of 64 sockets per thread for the fd_set. Not possible: The highest file descriptor ID counts, not the amount.
  • Use WSAPoll(), however that one has been added in Windows Vista and is therefore not available in XP, which might be a real showstopper (also it's Windows-only, on Linux you still have the 1024 limit (in detail it means the highest file descriptor ID must be less than 1024, not the amount!!))

Seems like wrapping the sockets API isn't enough. SFML Network probably needs another layer of abstraction if you want to keep it simple to use without annoying limitations.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 9, 2012

Member

So far nobody complained about the limitation, I guess that 64 sockets is enough for most applications.

Since it's potentially a complicated problem, and I have other priorities for the near future, I'll leave the implementation as it is with its limitations, and wait for more feedback with real use cases.

Thank you very much for your feedback and investigations!

Member

LaurentGomila commented Jan 9, 2012

So far nobody complained about the limitation, I guess that 64 sockets is enough for most applications.

Since it's potentially a complicated problem, and I have other priorities for the near future, I'll leave the implementation as it is with its limitations, and wait for more feedback with real use cases.

Thank you very much for your feedback and investigations!

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jan 9, 2012

Member

You're welcome, but please at least add an assert() or debug message to Selector::add() that warns the user when a file descriptor with an ID >= FD_SETSIZE is being added (+ documentation). I think debugging such a bug is not so much fun.

Member

TankOs commented Jan 9, 2012

You're welcome, but please at least add an assert() or debug message to Selector::add() that warns the user when a file descriptor with an ID >= FD_SETSIZE is being added (+ documentation). I think debugging such a bug is not so much fun.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jan 9, 2012

Member

please at least add an assert() or debug message to Selector::add() that warns the user when a file descriptor with an ID >= FD_SETSIZE is being added (+ documentation)

Sure.

Member

LaurentGomila commented Jan 9, 2012

please at least add an assert() or debug message to Selector::add() that warns the user when a file descriptor with an ID >= FD_SETSIZE is being added (+ documentation)

Sure.

@LaurentGomila LaurentGomila removed their assignment May 19, 2014

@TankOs TankOs added the s:unassigned label Jun 6, 2014

TankOs added a commit that referenced this issue Jun 9, 2014

Check socket descriptor limit. #153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message and an assert as well.

TankOs added a commit that referenced this issue Jun 9, 2014

Check socket descriptor limit. #153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message for this case.

TankOs added a commit that referenced this issue Jun 10, 2014

Check socket descriptor limit. #153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message for this case.

TankOs added a commit that referenced this issue Jun 11, 2014

Check socket descriptor limit. #153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message for this case.

TankOs added a commit that referenced this issue Jun 11, 2014

Check socket descriptor limit. #153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message for this case.

@TankOs TankOs self-assigned this Jun 11, 2014

@TankOs TankOs modified the milestones: 2.2, 2.x Jun 11, 2014

@TankOs TankOs added s:accepted and removed s:unassigned labels Jun 11, 2014

@TankOs TankOs closed this Jun 11, 2014

@Bromeon Bromeon added s:superseded and removed s:accepted labels Jun 11, 2014

MarioLiebisch added a commit to MarioLiebisch/SFML that referenced this issue Jun 13, 2014

Check socket descriptor limit. SFML#153
When calling select(), there's an upper limit for the socket descriptor
which is defined as FD_SETSIZE. When the socket descriptor is higher
than FD_SETSIZE, a call to select() will not work as expected, at least
for the proper sockets.

This patch adds an error message for this case.

@eXpl0it3r eXpl0it3r added s:accepted and removed s:superseded labels Jul 7, 2014

@LaurentGomila LaurentGomila reopened this Jul 15, 2014

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila
Member

LaurentGomila commented Jul 15, 2014

The applied patch is wrong on Windows:
http://en.sfml-dev.org/forums/index.php?topic=15815.0

binary1248 added a commit that referenced this issue Jul 17, 2014

Fixed SocketSelector not being able to accept sockets with IDs larger…
… than FD_SETSIZE on Windows (#153) and added the same checks to other affected methods as well.
@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 17, 2014

Member

Should be fixed in ddd259e.

Member

binary1248 commented Jul 17, 2014

Should be fixed in ddd259e.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 11, 2014

Member

This branch has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

Member

eXpl0it3r commented Aug 11, 2014

This branch has been added to my merge list, meaning it will be merged very soon, unless someone raises any concerns.

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Aug 11, 2014

Member

Don't we want to wait until the branch maintainer (binary1248 in this case) creates a pull request, or we have feedback from other people? By merging prematurely, we bypass the reviewing process.

The same applies to #412.

Member

Bromeon commented Aug 11, 2014

Don't we want to wait until the branch maintainer (binary1248 in this case) creates a pull request, or we have feedback from other people? By merging prematurely, we bypass the reviewing process.

The same applies to #412.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 11, 2014

Member

I didn't feel like opening up a pull request just to signify that the commit is ready for being reviewed/merged. If that was the case, I would prefer opening up the PR and closing original issue as superseded so we don't have to have 2 simultaneous issues dealing with the same matter.

Member

binary1248 commented Aug 11, 2014

I didn't feel like opening up a pull request just to signify that the commit is ready for being reviewed/merged. If that was the case, I would prefer opening up the PR and closing original issue as superseded so we don't have to have 2 simultaneous issues dealing with the same matter.

binary1248 added a commit that referenced this issue Aug 18, 2014

Fixed SocketSelector not being able to accept sockets with IDs larger…
… than FD_SETSIZE on Windows (#153) and added the same checks to other affected methods as well.
@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 18, 2014

Member

Merged in b96d330

Member

eXpl0it3r commented Aug 18, 2014

Merged in b96d330

@eXpl0it3r eXpl0it3r closed this Aug 18, 2014

@TankOs TankOs removed their assignment Apr 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment