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 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

@unasuke unasuke mentioned this pull request Dec 18, 2019
@unasuke unasuke force-pushed the unasuke:frozen_string_literal branch from 5b4b44e to e7b75bc Dec 18, 2019
@geemus geemus self-requested a review Dec 18, 2019
Copy link
Contributor

geemus left a comment

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.
@unasuke unasuke force-pushed the unasuke:frozen_string_literal branch from e7b75bc to 6609703 Dec 18, 2019
@unasuke unasuke requested a review from geemus Dec 18, 2019
@geemus
geemus approved these changes Dec 18, 2019
Copy link
Contributor

geemus left a comment

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

@geemus geemus merged commit ac85f68 into excon:master Dec 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geemus

This comment has been minimized.

Copy link
Contributor

geemus commented Dec 18, 2019

@unasuke do you need a release with these changes?

@unasuke unasuke deleted the unasuke:frozen_string_literal branch Dec 18, 2019
@unasuke

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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
Projects
None yet
2 participants
You can’t perform that action at this time.