Skip to content
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

Fix for when recv returns nil #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

paccator
Copy link

I was getting this error when connection can't be established:

NoMethodError: undefined method 'empty?' for nil
from .../lib/socksify/socksproxyable.rb:107:in 'Socksproxyable::InstanceMethodsConnect#socks_receive_reply'

I think it's related to a change in ruby 3.3:

BasicSocket#recv and BasicSocket#recv_nonblock returns nil instead of an empty string on closed connections.

https://rubyreferences.github.io/rubychanges/3.3.html#standard-library
https://bugs.ruby-lang.org/issues/19012


So here I propose to make sure we operate on a string, not nil.

In few places we could also just check for #nil? or empty? but #to_s in happy path is basically a noop.

Not much to add or change in tests.

Since ruby 3.3 BasicSocket#recv returns nil instead of an empty string on closed connections

https://rubyreferences.github.io/rubychanges/3.3.html#standard-library

https://bugs.ruby-lang.org/issues/19012
Copy link

nix-ci-bot bot commented Feb 11, 2025

NixCI is ready to run on this PR.

@astro
Copy link
Owner

astro commented Feb 11, 2025

Personal opinion: I wouldn't merge this because I like the semantics of "" meaning no more data and nil signifying a closed read half.

However, I've passed maintainership to others a long time ago and will accept their decision.

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

Successfully merging this pull request may close these issues.

2 participants