Skip to content
New issue

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

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

Already on GitHub? # to your account

Removes call to getpeername in SocketStream #744

Merged
merged 4 commits into from
Nov 4, 2018

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Oct 21, 2018

Fixes #609.

I have a few questions:

@njsmith
Copy link
Member

njsmith commented Oct 21, 2018

@wgwz
Copy link
Contributor Author

wgwz commented Oct 22, 2018

OK cleared up that test and added a newsfragment.

@codecov
Copy link

codecov bot commented Oct 22, 2018

Codecov Report

Merging #744 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #744      +/-   ##
==========================================
- Coverage   99.34%   99.32%   -0.02%     
==========================================
  Files          96       96              
  Lines       11577    11636      +59     
  Branches      827      832       +5     
==========================================
+ Hits        11501    11558      +57     
- Misses         56       57       +1     
- Partials       20       21       +1
Impacted Files Coverage Δ
trio/_highlevel_socket.py 100% <ø> (ø) ⬆️
trio/tests/test_highlevel_open_tcp_stream.py 100% <ø> (ø) ⬆️
trio/tests/test_highlevel_socket.py 100% <100%> (ø) ⬆️
trio/tests/test_exports.py 96.42% <0%> (-3.58%) ⬇️
trio/tests/test_ssl.py 100% <0%> (ø) ⬆️
trio/_wait_for_object.py 100% <0%> (ø) ⬆️
trio/tests/test_sync.py 100% <0%> (ø) ⬆️
trio/_sync.py 100% <0%> (ø) ⬆️
trio/_core/_exceptions.py 100% <0%> (ø) ⬆️
trio/socket.py 100% <0%> (ø) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 106991f...f2261ca. Read the comment docs.

@njsmith
Copy link
Member

njsmith commented Oct 23, 2018

This is pretty close! A few comments:

  • The codecov report is saying there are some new uncovered lines... it looks like it's this getpeername method and this getpeername method, which are little stubs that were only added to make SocketStream happy. Now that SocketStream doesn't care about getpeername, we should delete those methods too. (And I guess update this comment.)

  • FYI, you can use Sphinx markup in newsfragments – so instead of ``SocketStream``, it'd be more idiomatic to write :class:`SocketStream`.

  • In the contributing guide, we have a bit of discussion about the difference between a good release note and a good commit message. Right now, your newsfragment right now describes what you changed and why, and it would make a great commit message, because commit messages are targeted at other people working on Trio's code. But it's not a great release note, because it's not aimed at the average Trio user. Instead, I'd suggest using something like: "Fixed a race condition on macOS, where Trio's TCP listener would crash if an incoming TCP connection was closed before the listener had a chance to accept it."

@wgwz
Copy link
Contributor Author

wgwz commented Nov 4, 2018

It's nice how the codecov report revealed those issues! I took a look at the reports. Thanks for suggestions on the newsfragments, all points noted. :-)

Oh and sorry it took me so long to finish this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SocketStream.__init__'s check for whether the socket is connected is unreliable on MacOS
3 participants