Skip to content

bpo-37405: socket.getsockname() returns string instead of tuple #14392

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 12, 2019
Merged

bpo-37405: socket.getsockname() returns string instead of tuple #14392

merged 9 commits into from
Sep 12, 2019

Conversation

bggardner
Copy link
Contributor

@bggardner bggardner commented Jun 26, 2019 •
edited by bedevere-bot
Loading

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

tirkarthi
tirkarthi reviewed Jun 26, 2019
View reviewed changes
Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test at

cpython/Lib/test/test_socket.py

Line 1923 in c6a2320

s.bind(('', ))
to test for s.getsockname() to return a tuple instead of a string. I think this also requires a NEWS entry since there is change in the type of return value.

@bggardner
Copy link
Contributor Author

bggardner commented Jun 26, 2019 •
edited
Loading

A couple questions, as I am new to contributing:

  1. This fix reverts to the previously returned data type of a tuple, which matches the documentation. There wasn't a NEWS.d update about changing the data type in the original change. So, can a "skip news" label be applied?

  2. The documentation for socket.getsockname() says the returned data type depends on the address family. Therefore, I assume that it will return the same data type that is required for the address arguments of bind(), connect(), etc. If this assumption is correct, then should a test that takes the output of socket.getsockname() and feed it to socket.bind() be applied to every family instead of just AF_CAN? In the meantime, I'll just add a testBind(self) method to the BasicCANTest class, similar to others which check the output of getsockname().

@tirkarthi
Copy link
Member

Sorry, I thought this was fixing a regression. Looking at the commit I am not sure if it was intentional or missed to return a string. I would wait for a core dev input on this.

@pylessard
Copy link
Contributor

Hi, to answer your question, no, this was not intentional.
I do believe that the getsockname() output should match the .bind() interface though. If we do s.bind('can0', ) then I think s.getsockname() should return ('can0',) not ('can0', socket.AF_CAN)

@bggardner
Copy link
Contributor Author

Agreed. I never understood why socket.AF_CAN was included in the tuple, so I'll update the pull request with @pylessard 's suggestion.

benjaminp
benjaminp requested changes Sep 11, 2019
View reviewed changes
Copy link
Contributor

@benjaminp benjaminp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have NEWS note because the regression was released.

Lib/test/test_socket.py Outdated
@@ -1921,6 +1921,7 @@ def testCreateBCMSocket(self):
def testBindAny(self):
with socket.socket(socket.PF_CAN, socket.SOCK_RAW, socket.CAN_RAW) as s:
s.bind(('', ))
self.assertIsInstance(s.getsockname(), tuple)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to make the more specific assertion here that s.getsockname() == ('',)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so, even though it isn't the greatest of tests to begin with. I'll make the change anyway.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@bggardner
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@benjaminp: please review the changes made to this pull request.

@benjaminp benjaminp merged commit 954900a into python:master Sep 12, 2019
@miss-islington
Copy link
Contributor

Thanks @bggardner for the PR, and @benjaminp for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 12, 2019
@bggardner @miss-islington
closes bpo-37405: Make socket.getsockname() always return a tuple for…
8d3676d 
… AF_CAN. (pythonGH-14392)

This fixes a regression from 3.5. In recent releases, `getsockname()` in the AF_CAN case has returned a string.
(cherry picked from commit 954900a)

Co-authored-by: bggardner <brent@ebrent.net>
@bedevere-bot
Copy link

GH-16018 is a backport of this pull request to the 3.8 branch.

benjaminp pushed a commit that referenced this pull request Sep 12, 2019
@miss-islington @bggardner @benjaminp
closes bpo-37405: Make socket.getsockname() always return a tuple for…
f60fd95 
… AF_CAN. (GH-14392) (GH-16018)

This fixes a regression from 3.5. In recent releases, `getsockname()` in the AF_CAN case has returned a string.
(cherry picked from commit 954900a)

Co-authored-by: bggardner <brent@ebrent.net>
DinoV pushed a commit to DinoV/cpython that referenced this pull request Sep 12, 2019
@bggardner @DinoV
closes bpo-37405: Make socket.getsockname() always return a tuple for…
a29b0c6 
… AF_CAN. (pythonGH-14392)

This fixes a regression from 3.5. In recent releases, `getsockname()` in the AF_CAN case has returned a string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tirkarthi tirkarthi tirkarthi left review comments

@benjaminp benjaminp Awaiting requested review from benjaminp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bggardner @the-knights-who-say-ni @tirkarthi @pylessard @bedevere-bot @miss-islington @benjaminp