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

Check socket descriptor limit. #153 #628

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@TankOs
Member

TankOs commented Jun 9, 2014

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 TankOs added bug labels Jun 9, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 9, 2014

Member

Pushed revised patch.

Member

TankOs commented Jun 9, 2014

Pushed revised patch.

@TankOs TankOs removed p:android labels Jun 10, 2014

@TankOs TankOs self-assigned this Jun 10, 2014

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

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 10, 2014

Member

Requesting review/acceptance. :) Btw, this is also a candidate for throwing an exception in SFML 3.

Member

TankOs commented Jun 10, 2014

Requesting review/acceptance. :) Btw, this is also a candidate for throwing an exception in SFML 3.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 10, 2014

Member

I'd change two things:

  • in a if/else block, I usually put the regular code first, and the error handling in the 'else'.
  • the message is not very descriptive for regular users; something like "Maximum number of sockets reached in selector" may be more appropriate.
Member

LaurentGomila commented Jun 10, 2014

I'd change two things:

  • in a if/else block, I usually put the regular code first, and the error handling in the 'else'.
  • the message is not very descriptive for regular users; something like "Maximum number of sockets reached in selector" may be more appropriate.

@Bromeon Bromeon removed the s:undecided label Jun 10, 2014

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 10, 2014

Member

Will change the first, however the message exactly states what the real problem is, and googling for "limit FD_SETSIZE" will lead the user to the information that's needed to learn about the issue, possibly avoiding it. If the user however reads "Maximum number of sockets reached in selector", he might try to use another selector, or less sockets -- however this is not the issue, it's the descriptor's value itself that exceeds a limit.

Member

TankOs commented Jun 10, 2014

Will change the first, however the message exactly states what the real problem is, and googling for "limit FD_SETSIZE" will lead the user to the information that's needed to learn about the issue, possibly avoiding it. If the user however reads "Maximum number of sockets reached in selector", he might try to use another selector, or less sockets -- however this is not the issue, it's the descriptor's value itself that exceeds a limit.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 10, 2014

Member

Pushed changes.

Member

TankOs commented Jun 10, 2014

Pushed changes.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 10, 2014

Member

The convention is to use curly braces in the 'else' if the 'if' has them, even if it's just for one line of code :p

It's true that the message that I gave wasn't accurate, but can't we find something that would be both understandable by users and relevant/accurate about the technical limitation?

Member

LaurentGomila commented Jun 10, 2014

The convention is to use curly braces in the 'else' if the 'if' has them, even if it's just for one line of code :p

It's true that the message that I gave wasn't accurate, but can't we find something that would be both understandable by users and relevant/accurate about the technical limitation?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Your code style is driving me nuts. ;)

I can try to find another wording, but to be honest it will probably only lead to a more inaccurate version. The problem with this issue is very technical, and to fully understand it, one has to read about what a file descriptor is and how FD_SET works -- together with its limitations.

When I ran into the issue, I would have been very (very!) happy about an error message that I can simply google for, showing me exactly what the problem is.

But let's see if I can combine this with a more verbose version.

Member

TankOs commented Jun 11, 2014

Your code style is driving me nuts. ;)

I can try to find another wording, but to be honest it will probably only lead to a more inaccurate version. The problem with this issue is very technical, and to fully understand it, one has to read about what a file descriptor is and how FD_SET works -- together with its limitations.

When I ran into the issue, I would have been very (very!) happy about an error message that I can simply google for, showing me exactly what the problem is.

But let's see if I can combine this with a more verbose version.

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

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Pushed changes.

Member

TankOs commented Jun 11, 2014

Pushed changes.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Jun 11, 2014

Member

Perfect :p

Member

LaurentGomila commented Jun 11, 2014

Perfect :p

@TankOs TankOs closed this Jun 11, 2014

@TankOs TankOs deleted the bugfix/socket_assert branch Jun 11, 2014

@Bromeon Bromeon added the s:accepted label Jun 11, 2014

@Bromeon Bromeon added this to the 2.2 milestone Jun 11, 2014

@Bromeon

This comment has been minimized.

Show comment
Hide comment
@Bromeon

Bromeon Jun 11, 2014

Member

It's written in the title, but I'll mention the original issue #153 in the text, so that it will also have a reference to this PR.

Member

Bromeon commented Jun 11, 2014

It's written in the title, but I'll mention the original issue #153 in the text, so that it will also have a reference to this PR.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Jun 11, 2014

Member

Thanks!

Member

TankOs commented Jun 11, 2014

Thanks!

@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