Bug #15318
closed
- Subject changed from net/imap socket backward compatibility broken between in ruby 2.5+ to net/imap socket backward compatibility broken in ruby 2.5+
- Status changed from Open to Feedback
- Assignee set to shugo (Shugo Maeda)
Could you elaborate how it is broken?
nobu (Nobuyoshi Nakada) wrote:
Could you elaborate how it is broken?
Sure.
require 'net/imap'
require 'net/pop'
require 'socksify'
Socksify::debug = true
Socksify::proxy('127.0.0.1', 1080) do |_|
# in ruby 2.4.5p335 (2018-10-18 revision 65137) [x86_64-linux] connection will go throgh socks
# in ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux] connection will be from current host, omitting socks without any error or warning
imap = Net::IMAP.new('imap.host.com', 143)
# in ruby 2.4.5p335 (2018-10-18 revision 65137) [x86_64-linux] connection will go throgh socks
# in ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux] connection will go throgh socks
pop = Net::POP3.new('pop3.host.com', 110)
end
ok, IMAP uses Socket.tcp instead of TCPSocket, so if you patch TCPSocket directly it won't work anymore.
the intention was to get rid of Timeout.timeout {} and make these libraries fully async
there are simmilar pending changes for POP3 and other protocols:
https://bugs.ruby-lang.org/issues/12928
https://bugs.ruby-lang.org/issues/12435
I think the main blocker is that getaddrinfo isn't async and resolv.rb is much slower.
ahorek (Pavel Rosický) wrote:
ok, IMAP uses Socket.tcp instead of TCPSocket, so if you patch TCPSocket directly it won't work anymore.
the intention was to get rid of Timeout.timeout {} and make these libraries fully async
there are simmilar pending changes for POP3 and other protocols:
https://bugs.ruby-lang.org/issues/12928
https://bugs.ruby-lang.org/issues/12435
I think the main blocker is that getaddrinfo isn't async and resolv.rb is much slower.
I understand your intentions and don't know what are you going to do next, but it's not a good decision to create and then to break internal interfaces which is base for other programmers to override and extend standard library.
It wasn't intentional, but I think you shouldn't depend on TCPSocket anyway.
1/ Net::IMAP doesn't expose @socket in a public api
2/ it isn't documented that Net::IMAP will always use TCPSocket
@nobu (Nobuyoshi Nakada) what's your opinion? we should at least unify net/* libraries.
any idea how to help socksify to use this pattern without patching TCPSocket directly?
Socksify::proxy('127.0.0.1', 1080) do |_|
imap = Net::IMAP.new('imap.host.com', 143)
end
#12435 will also break other protocols and I think we can't avoid that without changing socksify's code...
ahorek (Pavel Rosický) wrote:
It wasn't intentional, but I think you shouldn't depend on TCPSocket anyway.
1/ Net::IMAP doesn't expose @socket in a public api
2/ it isn't documented that Net::IMAP will always use TCPSocket
One more comment about it if you don't mind. Internal interfaces being created for some reason, and there was a reason to create TCPSocket, such as proxification. We are talking about standard library, not ruby internal structures which is understandable can be rewritten and refactored. So it's normal to override and extend even standard library code through interface, it's how ruby offers code reuse and DRY. You are right, there is no documentation about TCPSocket and any guarantees it will remain so, but it's a bit late to break something after keeping it in a known way until 2.5 release. Thank you.
karis10@f5foster.com wrote:
https://bugs.ruby-lang.org/issues/15318
"Socket.tcp" been part of the public Ruby API for around 10 years
already. socksify should be supporting it in addition to TCPSocket;
because there is code which uses Socket directly.
It should've been wrapping all calls to Socket.new and maybe
Socket.for_fd to intercept the AF_INET* && SOCK_STREAM ones, too.
- Status changed from Feedback to Rejected
normalperson (Eric Wong) wrote:
"Socket.tcp" been part of the public Ruby API for around 10 years
already. socksify should be supporting it in addition to TCPSocket;
because there is code which uses Socket directly.
Agreed.
@sock is a private API and Socket is recommended over TCPSocket, so I wouldn't like to
revert the change.
Also available in: Atom
PDF
Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0