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

Explicit handling of frozen strings in IO::Buffer#for. #5892

Merged
merged 1 commit into from May 8, 2022

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented May 7, 2022

It turns out that we can't touch attributes during GC free, so unlocking the buffer's source during GC is undefined (in a bad way). So, we change the API for IO::Buffer#for slightly to avoid this case. It still works in the scenarios we expect, e.g.

Defining global constant buffers:

# frozen_string_literal: true (optional but slightly more efficient)

TOKEN = IO::Buffer.for("my-token") # TOKEN.readonly? => true
def manipulate(string)
  IO::Buffer.for(string) do |buffer|
    buffer.set_string("Hello World") # Mutate string.
  end
end

@ioquatix ioquatix force-pushed the io-buffer-mutable-string-source-fix branch 2 times, most recently from 565f6d7 to 68e58a4 Compare May 8, 2022 00:04
@ioquatix ioquatix requested a review from nobu May 8, 2022 00:05
@ioquatix
Copy link
Member Author

ioquatix commented May 8, 2022

rbs assumes the previous behaviour and it seems like we can't merge this PR until it is fixed: ruby/rbs#989

@ioquatix ioquatix force-pushed the io-buffer-mutable-string-source-fix branch from 68e58a4 to 311f6ac Compare May 8, 2022 01:15
@soutaro soutaro mentioned this pull request May 8, 2022
@ioquatix
Copy link
Member Author

ioquatix commented May 8, 2022

@soutaro looks good now!

@ioquatix ioquatix merged commit ef525b0 into ruby:master May 8, 2022
@ioquatix ioquatix deleted the io-buffer-mutable-string-source-fix branch May 8, 2022 23:03
@casperisfine
Copy link
Contributor

Is the backport label on GitHub enough? Or should we create a Redmine ticket (asking because a colleague just ran into this bug).

casperisfine pushed a commit to Shopify/ruby that referenced this pull request May 11, 2022
@jeremyevans
Copy link
Contributor

@casperisfine All backport requests should be submitted as closed bugs on Redmine with the appropriate backport field set to REQUIRED for the ruby versions you would like the fix backported to.

@casperisfine
Copy link
Contributor

@jeremyevans thank you for confirming, that's what I remembered. I'll create the backport issue then.

schneems pushed a commit to schneems/ruby that referenced this pull request May 23, 2022
schneems pushed a commit to schneems/ruby that referenced this pull request Jul 26, 2022
matzbot pushed a commit that referenced this pull request Sep 4, 2022
…Backport #18775]

	Explicit handling of frozen strings in `IO::Buffer#for`. (#5892)

	---
	 io_buffer.c                 | 122 +++++++++++++++++++++++++++++++++++---------
	 test/ruby/test_io_buffer.rb |  36 +++++++------
	 2 files changed, 117 insertions(+), 41 deletions(-)

	Fix rdoc of IO::Buffer [ci skip]

	---
	 io_buffer.c | 15 +--------------
	 1 file changed, 1 insertion(+), 14 deletions(-)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants