Skip to content

Improve socket specs#6711

Merged
RX14 merged 4 commits intocrystal-lang:masterfrom
straight-shoota:jm/feature/socket-specs-expand
Oct 10, 2018
Merged

Improve socket specs#6711
RX14 merged 4 commits intocrystal-lang:masterfrom
straight-shoota:jm/feature/socket-specs-expand

Conversation

@straight-shoota
Copy link
Copy Markdown
Member

@straight-shoota straight-shoota commented Sep 12, 2018 •
edited
Loading

Adds new and improves existing specs for Socket, TCPSocket, TCPServer, UNIXServer and UDPSocket.

The diff is best viewed with -w/&w=1 to ignore whitespace.

This is still work in progress, I'm publishing it as PR to validate if the specs perform correctly on all platforms.

@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 0168ab9 to cb7f8bf Compare September 12, 2018 22:53
@ysbaddaden
Copy link
Copy Markdown
Collaborator

ysbaddaden commented Sep 13, 2018

Some specs are lost. For example SO_REUSEPORT or ipv4 connections to ipv6 server running on ::.

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 13, 2018

ipv4 connections to ipv6 server running on ::.

Was there a explicit spec for that? I can't find it... But I can add it, of course.

@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 8508598 to 62b585e Compare September 13, 2018 09:49
@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 13, 2018 •
edited
Loading

The specs that are now failing might be bugs, but I'm not sure.

  1. Linux: When querying a previously set recv_buffer_size, a different value is returned.
  2. ALL: Connecting with an an IPv6 address (::1) to an IPv4 server (0.0.0.0) fails. Not sure if that is expected. The other way around (127.0.0.1 to ::) works.
  3. FreeBSD: Connecting with with 127.0.0.1 to :: fails.
  4. MacOS, FreeBSD: UDPSocket can't send broadcast to 255.255.255.255.

@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 62b585e to c771391 Compare September 13, 2018 10:00
@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 13, 2018

@straight-shoota connecting ipv6 to ipv4 is expected to fail. Binding to :: binds to v4 and v6, binding to 0.0.0.0 binds to v4 only. Depending on kernel options though, which might make this test brittle

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 13, 2018

So, it's probably better not to spec this?

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 13, 2018

The spec connecting with 127.0.0.1 to :: fails on FreeBSD.

@RX14
Copy link
Copy Markdown
Member

RX14 commented Sep 13, 2018 •
edited
Loading

@straight-shoota whether :: binds to ipv4 by default is OS-dependant and depends on sysctl values, see http://man7.org/linux/man-pages/man7/ipv6.7.html ctrl-f IPV6_V6ONLY. It's default value is controlled via sysctl. So if we could specifically specify this flag then we could spec the behaviour, but as it is, we leave this at default setting, so we shouldn't spec this.

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Sep 17, 2018

The two failing specs are now marked as pending and I'll open new issues for tracking them.
Now the specs compile on every platform.

The fails on travisci are unrelated to this PR.

@straight-shoota straight-shoota changed the title (WIP) Improve socket specs Improve socket specs Sep 17, 2018
@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 10629cb to 701e79f Compare September 17, 2018 16:22
@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 701e79f to 0ac35ce Compare October 2, 2018 18:29
@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Oct 3, 2018

Can this get a review, please?

@straight-shoota
Copy link
Copy Markdown
Member Author

straight-shoota commented Oct 4, 2018

I added two additional specs that came up during refactoring.

@straight-shoota straight-shoota force-pushed the jm/feature/socket-specs-expand branch from 5f74b9e to 5941ca5 Compare October 4, 2018 16:05
@RX14 RX14 requested review from bcardiff and ysbaddaden October 5, 2018 17:36
sdogruyol
sdogruyol approved these changes Oct 9, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

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

Thank you @straight-shoota 👍

@jkthorne
Copy link
Copy Markdown
Contributor

jkthorne commented Oct 9, 2018

Thank you @straight-shoota !

@RX14 RX14 added this to the 0.27.0 milestone Oct 10, 2018
@RX14 RX14 merged commit e02ccc9 into crystal-lang:master Oct 10, 2018
@straight-shoota straight-shoota deleted the jm/feature/socket-specs-expand branch October 10, 2018 20:48
@straight-shoota straight-shoota mentioned this pull request Oct 16, 2018
Closed
@straight-shoota straight-shoota mentioned this pull request Oct 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdogruyol sdogruyol sdogruyol approved these changes

@RX14 RX14 RX14 approved these changes

@bcardiff bcardiff Awaiting requested review from bcardiff

@ysbaddaden ysbaddaden Awaiting requested review from ysbaddaden

Assignees

No one assigned

Labels

kind:refactor topic:stdlib:specs topic:stdlib

Projects

None yet

Milestone

0.27.0

Development

Successfully merging this pull request may close these issues.

5 participants

@straight-shoota @ysbaddaden @RX14 @jkthorne @sdogruyol