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

Use String#dup in Excon::Utils#binary_encode for frozen string #711

Merged
merged 2 commits into from Dec 18, 2019

Conversation

unasuke
Copy link
Contributor

@unasuke unasuke commented Dec 18, 2019

Hi, I'm Itamae gem maintainer.
https://github.com/itamae-kitchen/itamae

probrem

I found itamae gem's CI failing on ruby-head (it will release as Ruby 2.7).
I investigated about that reason.
CI failing by excon called from docker-api gem.
https://travis-ci.org/itamae-kitchen/itamae/builds/625473573

11: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/docker-api-1.34.2/lib/docker/connection.rb:40:in `request'
10: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/connection.rb:275:in `request'
 9: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/base.rb:22:in `request_call'
 8: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/base.rb:22:in `request_call'
 7: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/base.rb:22:in `request_call'
 6: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/idempotent.rb:19:in `request_call'
 5: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/instrumentor.rb:34:in `request_call'
 4: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/mock.rb:57:in `request_call'
 3: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/middlewares/redirect_follower.rb:15:in `request_call'
 2: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/connection.rb:164:in `request_call'
 1: from /home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/utils.rb:15:in `binary_encode'
/home/travis/build/itamae-kitchen/itamae/vendor/bundle/ruby/2.7.0/gems/excon-0.71.0/lib/excon/utils.rb:15:in `force_encoding': can't modify frozen String: "" (FrozenError) (Excon::Error::Socket)

solution

Therefore, use String#dup in Excon::Utils#binary_encode for frozen string literal, and caller assign returned duplicated string to a variable.

Screenshot from 2019-12-18 17-56-49
https://travis-ci.org/itamae-kitchen/itamae/builds/626589627

Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the detailed report and fix. I just have one area I'd like to discuss, which I'll comment on directly. Thanks!

lib/excon/utils.rb Outdated Show resolved Hide resolved
Excon::Utils#binary_encode may receive frosen string.
Copy link
Contributor

@geemus geemus left a comment

Choose a reason for hiding this comment

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

Looks good, thanks again for the detailed report, fix, and quick update.

@geemus geemus merged commit ac85f68 into excon:master Dec 18, 2019
@geemus
Copy link
Contributor

geemus commented Dec 18, 2019

@unasuke do you need a release with these changes?

@unasuke unasuke deleted the frozen_string_literal branch December 18, 2019 16:45
@unasuke
Copy link
Contributor Author

unasuke commented Dec 18, 2019

@geemus Yes! I want a new release! 😆
But I'll leave it up to you if you busy. ☺️

@geemus
Copy link
Contributor

geemus commented Dec 18, 2019

No worries, if you didn't care I would probably just wait, but it's pretty easy to do.

Released in v0.71.1.

Thanks again!

@unasuke
Copy link
Contributor Author

unasuke commented Dec 19, 2019

Thank you too!

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.

None yet

2 participants