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

UdpSocket.hpp typo in comment for void unbind() #1121

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@Dka8
Contributor

Dka8 commented Aug 1, 2016

Function void unbind();
Commentary:
/// The port that the socket was previously using is immediately
/// available after this function is called.

Typo in word "available" contradicts with function's designation:
Should be:
/// The port that the socket was previously using is immediately
/// unavailable after this function is called.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 2, 2016

Member

I disagree, after a call to this method the port should be available to be used again, e.g., for another call to bind(port). What might be confusing is that we are not talking about the socket here.

Member

mantognini commented Aug 2, 2016

I disagree, after a call to this method the port should be available to be used again, e.g., for another call to bind(port). What might be confusing is that we are not talking about the socket here.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 2, 2016

Member

I guess one could add "available to bind again after this function is called" or something similar, to make the intend clearer.

Member

eXpl0it3r commented Aug 2, 2016

I guess one could add "available to bind again after this function is called" or something similar, to make the intend clearer.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 2, 2016

Member

I don't know -- I feel it would imply that the port is still somehow owned by SFML instead of being available system-wide.

Member

mantognini commented Aug 2, 2016

I don't know -- I feel it would imply that the port is still somehow owned by SFML instead of being available system-wide.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 2, 2016

Member

Well right now it's slightly ambiguous (given that we have a PR open), so by stating for what the port becomes available again, will make it less ambiguous. My text was just a suggestion, feel free to provide a better one.

Member

eXpl0it3r commented Aug 2, 2016

Well right now it's slightly ambiguous (given that we have a PR open), so by stating for what the port becomes available again, will make it less ambiguous. My text was just a suggestion, feel free to provide a better one.

@Dka8

This comment has been minimized.

Show comment
Hide comment
@Dka8

Dka8 Aug 2, 2016

Contributor

Thanks for reply! Yes, after a call unbind() we surely can call bind(port) with the same port socket was bound before.
But right after a call unbind() method our socket forgot about a port it was bound before, didn't it?

Contributor

Dka8 commented Aug 2, 2016

Thanks for reply! Yes, after a call unbind() we surely can call bind(port) with the same port socket was bound before.
But right after a call unbind() method our socket forgot about a port it was bound before, didn't it?

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 3, 2016

Member

@eXpl0it3r: Yep, it indeed needs clarification. But maybe there's more that need to be fixed. I quickly read through the doc and examples. Despite being clear that bind must be called in order to receive data from a given port, the UDP client example doesn't do so (it works, at least locally on my machine). It would be better too if the two receive methods made a reference to bind and this requirement. Maybe this paragraph could be improved as well to make things more explicit.

But right after a call unbind() method our socket forgot about a port it was bound before, didn't it?

Yes, the port is available for a different usage. The socket is simply closed and next time it is used it will be re-opened -- well, that's what the implementation tells me.

It has been a while since I've worked with networking stuff, so maybe @LaurentGomila or @binary1248 have some comments on this?

Member

mantognini commented Aug 3, 2016

@eXpl0it3r: Yep, it indeed needs clarification. But maybe there's more that need to be fixed. I quickly read through the doc and examples. Despite being clear that bind must be called in order to receive data from a given port, the UDP client example doesn't do so (it works, at least locally on my machine). It would be better too if the two receive methods made a reference to bind and this requirement. Maybe this paragraph could be improved as well to make things more explicit.

But right after a call unbind() method our socket forgot about a port it was bound before, didn't it?

Yes, the port is available for a different usage. The socket is simply closed and next time it is used it will be re-opened -- well, that's what the implementation tells me.

It has been a while since I've worked with networking stuff, so maybe @LaurentGomila or @binary1248 have some comments on this?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 3, 2016

Member

To make it even less ambiguous, I would prefer something like:

/// The port that the socket was previously bound to is immediately
/// made available to the operating system after this function is called.
/// This means that a subsequent call to bind() will be able to re-bind
/// the port if no other process has done so in the mean time.

The reason why the UDP client example works is because we sent something prior to receiving. It is perfectly normal to fire single-shot datagrams at whatever destination without caring about a response, thus sending does not require binding to a known port if it is not necessary. In this case, the operating system temporarily binds the socket to an ephemeral port (those really huge ones) since it assumes the socket/connection is going to be short-lived. If the server responds fast enough using the sender address/port they will manage to get their reply datagram routed to the right process using the ephemeral port. This is what happens in the example, and sure... it might be counter-intuitive at first sight to someone learning networking while using SFML, but for someone who has worked with UDP before it is completely normal to write clients like that.

Member

binary1248 commented Aug 3, 2016

To make it even less ambiguous, I would prefer something like:

/// The port that the socket was previously bound to is immediately
/// made available to the operating system after this function is called.
/// This means that a subsequent call to bind() will be able to re-bind
/// the port if no other process has done so in the mean time.

The reason why the UDP client example works is because we sent something prior to receiving. It is perfectly normal to fire single-shot datagrams at whatever destination without caring about a response, thus sending does not require binding to a known port if it is not necessary. In this case, the operating system temporarily binds the socket to an ephemeral port (those really huge ones) since it assumes the socket/connection is going to be short-lived. If the server responds fast enough using the sender address/port they will manage to get their reply datagram routed to the right process using the ephemeral port. This is what happens in the example, and sure... it might be counter-intuitive at first sight to someone learning networking while using SFML, but for someone who has worked with UDP before it is completely normal to write clients like that.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 3, 2016

Member

Thanks @binary1248 for the explanation! The suggested text is indeed better. 👍

Member

mantognini commented Aug 3, 2016

Thanks @binary1248 for the explanation! The suggested text is indeed better. 👍

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 7, 2016

Member

@Dka8, could you update this and squash all commits into one? Thanks! :-)

Member

mantognini commented Aug 7, 2016

@Dka8, could you update this and squash all commits into one? Thanks! :-)

@Dka8

This comment has been minimized.

Show comment
Hide comment
@Dka8

Dka8 Aug 8, 2016

Contributor

@mantognini sorry, haven't been here for a while. It's done now!
@binary1248 Thank you! That was very educational! 👍

Contributor

Dka8 commented Aug 8, 2016

@mantognini sorry, haven't been here for a while. It's done now!
@binary1248 Thank you! That was very educational! 👍

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 9, 2016

Member

Thanks @Dka8!

Two remarks:

  • do we want to keep If the socket is not bound to a port, this function has no effect.?
  • the commits'll need to be squashed.
Member

mantognini commented Aug 9, 2016

Thanks @Dka8!

Two remarks:

  • do we want to keep If the socket is not bound to a port, this function has no effect.?
  • the commits'll need to be squashed.

@mantognini mantognini modified the milestone: 2.4.1 Aug 9, 2016

UdpSocket.hpp typo in comment for void unbind()
Function  void unbind();
The suggested text by @binary1248
Added "If the socket is not bound to a port, this function has no effect."
@Dka8

This comment has been minimized.

Show comment
Hide comment
@Dka8

Dka8 Aug 9, 2016

Contributor

@mantognini Sorry!
Line If the socket is not bound to a port, this function has no effect. now added and all commits are squashed.

Contributor

Dka8 commented Aug 9, 2016

@mantognini Sorry!
Line If the socket is not bound to a port, this function has no effect. now added and all commits are squashed.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Aug 9, 2016

Member

Wonderful!

Member

mantognini commented Aug 9, 2016

Wonderful!

@mantognini mantognini added s:accepted and removed s:undecided labels Aug 9, 2016

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 10, 2016

Member

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

Member

eXpl0it3r commented Aug 10, 2016

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

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 13, 2016

Member

Merged in cb77f4d on the 2.4.x branch

Member

eXpl0it3r commented Aug 13, 2016

Merged in cb77f4d on the 2.4.x branch

@eXpl0it3r eXpl0it3r closed this Aug 13, 2016

@eXpl0it3r eXpl0it3r self-assigned this Aug 13, 2016

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