-
-
Notifications
You must be signed in to change notification settings - Fork 396
[Ruby 3.2] Enable most tests for Socket::AF_UNIX and test for File.socket? on Windows #1300
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
base: master
Are you sure you want to change the base?
Conversation
f3ec3c8 to
f87cf33
Compare
51d1c32 to
8092a25
Compare
| @path1 = SocketSpecs.socket_path | ||
| @path2 = SocketSpecs.socket_path.chop + '2' | ||
| rm_r(@path2) | ||
|
|
||
| @client_raw = Socket.new(:UNIX, :DGRAM) | ||
| @client_raw.bind(Socket.sockaddr_un(@path1)) | ||
| @client_raw = Socket.new(:UNIX, :DGRAM) | ||
| @client_raw.bind(Socket.sockaddr_un(@path1)) | ||
|
|
||
| @server_raw = Socket.new(:UNIX, :DGRAM) | ||
| @server_raw.bind(Socket.sockaddr_un(@path2)) | ||
| @server_raw = Socket.new(:UNIX, :DGRAM) | ||
| @server_raw.bind(Socket.sockaddr_un(@path2)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should be possible to rewrite this using :STREAM sockets but just changing the type doesn't work.
library/socket/shared/address.rb
Outdated
| else | ||
| # see https://bugs.ruby-lang.org/issues/21702 | ||
| @addr.unix_path.should == @b.local_address.unix_path | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a special guard ruby_bug for such cases. It will not run a guarded test case on MRI but will on other Ruby implementations
From the contributing guide (#13669 is an id of the corresponding reported issue):
ruby_bug '#13669', ''...'3.2' do
it "works like this" do
# Specify the expected behavior here, not the bug
end
endThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, forgot about it. Though, the guide says "If [bugfix] is not backported or not likely, use ruby_version_is instead." (I don't understand the point about backporting? It will still be some minimum version.), and in any case version to expect the fix in is unknown currently. Does this PR need to be parked until someone can get on the bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruby_bug guard indicates that there is no any value for alternative Ruby implementations to follow the current CRuby's incorrect behaviour. If an issue isn't being fixed in CRuby and backported to the supported versions - this behaviour becomes a feature so alternative implementations need to implement it as well.
In this case it's definitely a bug and I suppose it will be fixed very soon.
Does this PR need to be parked until someone can get on the bug?
No.
As I wrote above the ruby_bug guard prevents running this case on CRuby so CI will be green. After fixing and backporting the guard is supposed to be removed. And the next release version (4.0) can be used temporary as a fixed Ruby version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification!
| data = @client.recvfrom(6) | ||
| (data in ["barfoo", ["AF_UNIX", String]]).should be_true | ||
| sock.close | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto ruby_bug
| data.should == ["barfoo", ["AF_UNIX", @server.local_address.unix_path]] | ||
| sock.close | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: may be it makes sense to have separate cases for recvfrom called on a client and on a server socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've separated them, but can't say that it is definitely better. It's interesting that tests for recvfrom on other socket types usually just ignore the address part, so who knows what lurks there.
| end | ||
|
|
||
| platform_is :linux do | ||
| # It isn't clear which platforms support remote addresses here, so only Linux tests this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK to remove the guard. If it passes on CRuby CI then it's completely fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It won't pass, as at least OSX does not implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. It may be better to specify it explicitly with platform_is_not :windows, :darwin (as far as CRuby's CI uses only Linux, macOS and Windows). If specs are run on other platforms I suppose mainterners of Ruby package will let us know that there are failing tests.
| # The second part of the address is a memory dump on Windows! | ||
| # See https://bugs.ruby-lang.org/issues/21702 | ||
| (@server.recvfrom(5) in ['hello', ['AF_UNIX', String]]).should be_true | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
8092a25 to
ba50fc1
Compare
Previously, it was just a fancy :windows platform guard. Unix sockets are now mostly implemented on Windows, making the feature useless.
ba50fc1 to
1d0c365
Compare
From #1016
File.socket?FileTest.socket?is actually tested.#to_path.Following things are not supported so the tests are disabled:
#send_io,#recv_io,#getpeerid(UNIX IDs),Socket pairs are emulated using a temporary file (ruby/ruby#6513 (comment)), so the behavior is different. Trying to create a Socket using a non-existent path raises a different error for whatever reason.
There are a couple bugs, reported as https://bugs.ruby-lang.org/issues/21702:
#remote_address.to_sreturns 110 bytes always;#recvfromreturns 2047 bytes of garbage as the remote address.