Skip to content

Represent empty body response as blank string. #47

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

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

ebababi
Copy link
Contributor

@ebababi ebababi commented Jan 22, 2025

Faraday adapters are expected to represent empty body response as blank string and this PR aligns async-http-faraday to this specification, fixing #46.

Moreover, the returned blank string is unfreezed, in the same way the "reference implementation" of the faraday-net_http adapter is doing it.

Last, here's a third-party implementation that I've stumbled upon that uses Faraday through the ruby-openai gem and always expects a string response.

Types of Changes

  • Bug fix.
  • Breaking change.

Not sure how you would categorize this change.

Contribution

@ioquatix
Copy link
Member

@iMacTia what about allowing the response body to be frozen, it's more efficient - immutable should be the default if possible.

@ioquatix ioquatix merged commit ef85c56 into socketry:main Jan 23, 2025
17 of 20 checks passed
@iMacTia
Copy link

iMacTia commented Jan 24, 2025

The reason we need it unfrozen is because we need to control the encoding of the string, BUT you should be able to return a frozen string with no issues: we check if the body is frozen shortly afterwards and call #dup on it if that's the case (see here).

Returning an already unfrozen string might be more efficient though to avoid Faraday dup'ing a really big response body

@ioquatix
Copy link
Member

ioquatix commented Jan 25, 2025

body.dup should be efficient for frozen strings as it should internally use copy-on-write.

@iMacTia
Copy link

iMacTia commented Jan 28, 2025

In that case then I'd expect you to be able to set the body to a frozen string and Faraday should just #dup it when needed

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