accept string based port for pack_sockaddr_in #85

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@geemus
Contributor

geemus commented Oct 3, 2011

MRI/Rubinius accept string based ports for sockaddr_in and convert appropriately, seems like jruby probably should as well.

@@ -510,7 +510,7 @@ public class RubySocket extends RubyBasicSocket {
@JRubyMethod(name = {"pack_sockaddr_in", "sockaddr_in"}, meta = true)
public static IRubyObject pack_sockaddr_in(ThreadContext context, IRubyObject recv, IRubyObject port, IRubyObject host) {
return pack_sockaddr_in(context, recv,
- RubyNumeric.fix2int(port),
+ RubyNumeric.fix2int(Integer.parseInt(port)),
host.isNil() ? null : host.convertToString().toString());

This comment has been minimized.

Show comment Hide comment
@nicksieger

nicksieger Oct 3, 2011

Member

What is the MRI/Rubinius logic for converting? We should probably do that rather than relying on RubyObject.toString(). It might work here, but we should be more explicit about the conversion.

@nicksieger

nicksieger Oct 3, 2011

Member

What is the MRI/Rubinius logic for converting? We should probably do that rather than relying on RubyObject.toString(). It might work here, but we should be more explicit about the conversion.

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 3, 2011

Contributor

I'm not entirely sure, not so experienced/great at trying to understand the c code. Here are the areas in question though, so far as I can tell:

MRI: https://github.com/ruby/ruby/blob/trunk/ext/socket/raddrinfo.c#L343
Rubinius: https://github.com/rubinius/rubinius/blob/master/lib/socket.rb#L397

I'd just try to fix it, but from looking at those and the pointer stuff, etc I'm not entirely sure what the answer is.

@geemus

geemus Oct 3, 2011

Contributor

I'm not entirely sure, not so experienced/great at trying to understand the c code. Here are the areas in question though, so far as I can tell:

MRI: https://github.com/ruby/ruby/blob/trunk/ext/socket/raddrinfo.c#L343
Rubinius: https://github.com/rubinius/rubinius/blob/master/lib/socket.rb#L397

I'd just try to fix it, but from looking at those and the pointer stuff, etc I'm not entirely sure what the answer is.

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 9, 2011

Member

Looks like MRI is converting to char* https://github.com/ruby/ruby/blob/trunk/ext/socket/raddrinfo.c#L315-316, and Rubinius is most definitely calling port.to_s, so this should be fine.

@BanzaiMan

BanzaiMan Oct 9, 2011

Member

Looks like MRI is converting to char* https://github.com/ruby/ruby/blob/trunk/ext/socket/raddrinfo.c#L315-316, and Rubinius is most definitely calling port.to_s, so this should be fine.

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 20, 2011

Member

I merged the patch to master in 18a02a9, to 1.6 branch in 4fd3100.

Member

BanzaiMan commented Oct 20, 2011

I merged the patch to master in 18a02a9, to 1.6 branch in 4fd3100.

@BanzaiMan BanzaiMan closed this Oct 20, 2011

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 21, 2011

Contributor

Turns out I still don't know java. I can't get things to compile even with my changes reverted on my machine. I'm just going to open a bug and leave it to you guys.

Contributor

geemus commented Oct 21, 2011

Turns out I still don't know java. I can't get things to compile even with my changes reverted on my machine. I'm just going to open a bug and leave it to you guys.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 21, 2011

Contributor

Actually, I can't figure out how to create issues either. Maybe this just isn't a good week for me. Anyway, would be nice if you could fix this. I had to code around it by adding my own conversion in something, which is suboptimal.

Contributor

geemus commented Oct 21, 2011

Actually, I can't figure out how to create issues either. Maybe this just isn't a good week for me. Anyway, would be nice if you could fix this. I had to code around it by adding my own conversion in something, which is suboptimal.

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 21, 2011

Member

geemus,

You'll have to register on codehaus in order to open a ticket. If you can describe the problem with a test case, I'm sure we can work this out.

Thank you for your time and effort.

Member

BanzaiMan commented Oct 21, 2011

geemus,

You'll have to register on codehaus in order to open a ticket. If you can describe the problem with a test case, I'm sure we can work this out.

Thank you for your time and effort.

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 24, 2011

Contributor
Contributor

geemus commented Oct 24, 2011

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 24, 2011

Member

It seems to me that JRuby is behaving exactly the same as MRI:

[system] ~ $ jruby -S irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - Connection refused
    from org/jruby/ext/socket/RubyTCPSocket.java:125:in `initialize'
    from org/jruby/RubyIO.java:868:in `new'
    from (irb):2:in `evaluate'
    from org/jruby/RubyKernel.java:1011:in `eval'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:158:in `eval_input'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:271:in `signal_status'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:155:in `eval_input'
    from org/jruby/RubyKernel.java:1337:in `loop'
    from org/jruby/RubyKernel.java:1115:in `catch'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:154:in `eval_input'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:71:in `start'
    from org/jruby/RubyKernel.java:1115:in `catch'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:70:in `start'
    from /Users/asari/Development/src/jruby/bin/jirb:13:in `(root)'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x1436ae83>

[system] ~ $ jruby --1.9 -S irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - Connection refused
    from org/jruby/ext/socket/RubyTCPSocket.java:125:in `initialize'
    from org/jruby/RubyIO.java:868:in `new'
    from (irb):2:in `evaluate'
    from org/jruby/RubyKernel.java:1016:in `eval'
    from org/jruby/RubyKernel.java:1337:in `loop'
    from org/jruby/RubyKernel.java:1129:in `catch'
    from org/jruby/RubyKernel.java:1129:in `catch'
    from /Users/asari/Development/src/jruby/bin/jirb:13:in `(root)'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x3c9ed91f>


[system] ~ $ irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - connect(2)
    from (irb):2:in `initialize'
    from (irb):2:in `new'
    from (irb):2
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x10ac658f8>

[system] ~ $ irb1.9
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - connect(2)
    from (irb):2:in `initialize'
    from (irb):2:in `new'
    from (irb):2
    from /usr/local/bin/irb1.9:12:in `<main>'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:fd 7>
irb(main):004:0> 

Did I miss something?

Member

BanzaiMan commented Oct 24, 2011

It seems to me that JRuby is behaving exactly the same as MRI:

[system] ~ $ jruby -S irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - Connection refused
    from org/jruby/ext/socket/RubyTCPSocket.java:125:in `initialize'
    from org/jruby/RubyIO.java:868:in `new'
    from (irb):2:in `evaluate'
    from org/jruby/RubyKernel.java:1011:in `eval'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:158:in `eval_input'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:271:in `signal_status'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:155:in `eval_input'
    from org/jruby/RubyKernel.java:1337:in `loop'
    from org/jruby/RubyKernel.java:1115:in `catch'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:154:in `eval_input'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:71:in `start'
    from org/jruby/RubyKernel.java:1115:in `catch'
    from /Users/asari/Development/src/jruby/lib/ruby/1.8/irb.rb:70:in `start'
    from /Users/asari/Development/src/jruby/bin/jirb:13:in `(root)'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x1436ae83>

[system] ~ $ jruby --1.9 -S irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - Connection refused
    from org/jruby/ext/socket/RubyTCPSocket.java:125:in `initialize'
    from org/jruby/RubyIO.java:868:in `new'
    from (irb):2:in `evaluate'
    from org/jruby/RubyKernel.java:1016:in `eval'
    from org/jruby/RubyKernel.java:1337:in `loop'
    from org/jruby/RubyKernel.java:1129:in `catch'
    from org/jruby/RubyKernel.java:1129:in `catch'
    from /Users/asari/Development/src/jruby/bin/jirb:13:in `(root)'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x3c9ed91f>


[system] ~ $ irb
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - connect(2)
    from (irb):2:in `initialize'
    from (irb):2:in `new'
    from (irb):2
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:0x10ac658f8>

[system] ~ $ irb1.9
irb(main):001:0> require 'socket'
=> true
irb(main):002:0> TCPSocket.new('localhost', '55')
Errno::ECONNREFUSED: Connection refused - connect(2)
    from (irb):2:in `initialize'
    from (irb):2:in `new'
    from (irb):2
    from /usr/local/bin/irb1.9:12:in `<main>'
irb(main):003:0> TCPSocket.new('localhost', '22')
=> #<TCPSocket:fd 7>
irb(main):004:0> 

Did I miss something?

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 24, 2011

Contributor

No, sorry. I think I missed the ball. I mistakenly thought that Socket.new would end up using Socket.sockaddr_in internally. I've added a test that does it directly (which fails in jruby but passes elsewhere, I believe). Sorry for all the confusion. Here is my latest (hopefully successful) attempt:
https://github.com/geemus/jruby/commit/71b463320398d14490f8545f0dcc1db084084a8c

Contributor

geemus commented Oct 24, 2011

No, sorry. I think I missed the ball. I mistakenly thought that Socket.new would end up using Socket.sockaddr_in internally. I've added a test that does it directly (which fails in jruby but passes elsewhere, I believe). Sorry for all the confusion. Here is my latest (hopefully successful) attempt:
https://github.com/geemus/jruby/commit/71b463320398d14490f8545f0dcc1db084084a8c

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 25, 2011

Member

Confirmed the test. Yay!

Member

BanzaiMan commented Oct 25, 2011

Confirmed the test. Yay!

BanzaiMan added a commit that referenced this pull request Oct 25, 2011

BanzaiMan added a commit that referenced this pull request Oct 25, 2011

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 25, 2011

Member

I pushed the modified fix for the master (fd90c1b) and the 1.6 branch (9dcd388).

Member

BanzaiMan commented Oct 25, 2011

I pushed the modified fix for the master (fd90c1b) and the 1.6 branch (9dcd388).

@BanzaiMan

This comment has been minimized.

Show comment Hide comment
@BanzaiMan

BanzaiMan Oct 25, 2011

Member

I also added a spec: rubyspec/rubyspec@7648a35

Member

BanzaiMan commented Oct 25, 2011

I also added a spec: rubyspec/rubyspec@7648a35

@geemus

This comment has been minimized.

Show comment Hide comment
@geemus

geemus Oct 25, 2011

Contributor

Thanks for helping out on this, I had hoped to take care of it myself, but it ended up being a bit harder than I had originally suspected.

Contributor

geemus commented Oct 25, 2011

Thanks for helping out on this, I had hoped to take care of it myself, but it ended up being a bit harder than I had originally suspected.

eregon added a commit that referenced this pull request Jun 29, 2015

Squashed 'spec/ruby/' changes from d128ccf..0957d24
0957d24 add BigDecimal#round special value specs
dc36858 Fix spec title
e145d75 Fix ARGF.seek spec
a27e082 Add spec for Comparable#== without #<=>.
7bbd215 Add a couple specs for constant resolution under module_eval
0afb501 Add spec for resolving constants in Module#module_eval
002731e Fix assertion in Module#module_eval.
e450b4a Add spec for redo in a method
63603b5 Add spec for next in a method
09db199 Add spec for spawn redirect to [file,mode].
071a996 Merge pull request #90 from ruby/time
fdf1ced Fix typos
f23b1cb Separate Time#<=> specs for millisecond vs microsecond precision
b0a3bd4 Remove duplicated assertion
541d1f7 Add spec for ARGF.argv
a0a5ca8 Mention it "needs to be reviewed for spec completeness"
4d680ed Remove extra backslash
c6558a5 Merge pull request #88 from JuanitoFatas/feature/string-concat-by-spaces
0258119 Update spec description for string literals
bca9b5f Add two specs for string concatenations by spaces
e9e02f5 Deduplicate example names
4355c35 Merge pull request #87 from JuanitoFatas/string/unicode-normalize
8626efe Fix unicode normalized spec
f1de617 Remove unused IO in File#read spec
293f6e4 Fix variable name in FIle#open
62a7b2c Prefer setting autoclose to close + rescue
d46d063 mkspec -c String; Copy spec content to mkspec generated spec
50fade7 Apply code review changes
7b6ce1f Try to clarify specs using the map_fd fixture.
7747042 General cleanup of IO specs
850f3ed Reorganize IO#popen specs
b8dd40b Make sure the old IO is closed in IO#initialize spec
136b80e Remove 1.8 readlines specs
21b694d No need to check for closed? since IO#read examples never close themselves
39d00ff Just use autoclose=false in socket for_fd specs
f58eb48 No need to re-assign @fd in IO.sysopen spec
b5aeacb Fix typo in CONTRIBUTING.md
93bc817 Improve notes about mkspec
43685be [core/string] Add String#unicode_normalize specs
bb3e13d SVG Travis badge to please the eyes 👀
ea9f106 Reorganize specs for multiple unnamed arguments.
c95400b Merge pull request #83 from JuanitoFatas/proc/arguments
5c9b2f1 [Language/block_spec] Add specs to check SyntaxError for block arguments
113e7e6 Merge pull request #85 from ruby/use-standard-require
24d31d1 Standardize require line for unboundmethod/shared/to_s.rb
6f18131 update MSpec
4527d8d The fixture helper now resolves real paths automatically
5aa9134 Fix duplication with CODE_LOADING_DIR
21dae98 Do not use the fixture helper on already expanded path
4c4523c Just check for realpath in spec_helper.rb
4dea397 Resolve realpath to fixture directory in -C spec
a03ed0e Have a better check for CODE_LOADING_DIR and realpath
a8884b9 Update required MSpec version
c1ab544 Update instructions to directly use a MSpec checkout
186300a Remove $SAFE-related specs.
b762526 Merge pull request #82 from ruby/fix-ruby-version
bc11d67 fixed wrong version of ruby
2b8e36b Merge pull request #81 from ruby/remove-safe3
7c640e2 discarded =3 example with Ruby 2.3
19af98c Improve a bit Array#bsearch_index spec
4e1a9b3 Merge pull request #78 from JuanitoFatas/feature/bsearch_index_spec
c5374cd Merge pull request #79 from JuanitoFatas/feature/bundled-with
18dd536 Clearer description of @array.bsearch_index { |x| (1 - x / 4) * (2**100)
ea9bd82 Remove specs of break
de6124c Add BUNDLED WITH to Gemfile.lock
7732e00 Fix example descriptions and reorganize specs
ac55574 Fix ARGF spec for Windows
2cab1f3 Fix syntax errors
0cf2e94 Port Array#bsearch_index spec from ruby/ruby@c64d283

git-subtree-dir: spec/ruby
git-subtree-split: 0957d245b6170480c880a62da19a335113cc8c84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment