Skip to content

Clarify use of @buffered and only update content-length when #finish.#2149

Merged
ioquatix merged 1 commit into
rack:mainfrom
socketry:rack-response-buffered
Jun 8, 2024
Merged

Clarify use of @buffered and only update content-length when #finish.#2149
ioquatix merged 1 commit into
rack:mainfrom
socketry:rack-response-buffered

Conversation

@ioquatix

@ioquatix ioquatix commented Jan 30, 2024

Copy link
Copy Markdown
Member

See #2148 for background context.

This change adjusts Rack::Response so that the internal @length variable is used until the request is finished, at which point @headers['content-length'] is added if appropriate. This simplifies the internal logic and avoids any potential synchronisation issues.

SirJosh1987

This comment was marked as off-topic.

@jeremyevans

Copy link
Copy Markdown
Contributor

I think this can be closed. It was a bug fix for #2148, which was fixed by #2150

@ioquatix ioquatix force-pushed the rack-response-buffered branch from d48b4ad to f614053 Compare June 7, 2024 08:22
@ioquatix

ioquatix commented Jun 7, 2024

Copy link
Copy Markdown
Member Author

I think we should merge this. It's a simplification to the way the response length is handled. Instead of updating @length and @headers['content-length'] and trying to keep them in sync, this PR only updates @length and adds @headers['content-length'] if appropriate when "finishing" the response, which makes the most sense to me. Repeatedly updating @headers['content-length'] also seems pretty inefficient to me.

@ioquatix ioquatix modified the milestone: v3.1.0 Jun 7, 2024
@ioquatix ioquatix merged commit a057e9b into rack:main Jun 8, 2024
@ioquatix ioquatix deleted the rack-response-buffered branch June 8, 2024 01:25
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.

3 participants