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

Consider interacting with the network off the calling thread #313

Closed
jzbrooks opened this issue Aug 5, 2024 · 5 comments
Closed

Consider interacting with the network off the calling thread #313

jzbrooks opened this issue Aug 5, 2024 · 5 comments

Comments

@jzbrooks
Copy link
Contributor

jzbrooks commented Aug 5, 2024

When retrofit added suspend support, its suspending functions ran the actual network interaction off the calling thread. While it's fairly easy to wrap connect-kotlin generated suspend calls in a withContext dispatch, having that thread switch happen automatically seems less error prone. Since we use both buf and retrofit in our codebase, this is a relatively common source of confusion that often results in NetworkOnMainThreadExceptions.

Square's implementation uses their own executor, but using the built-in Dispatchers.IO might work too? I'm unsure which would be preferable.

square/retrofit#2886

@jhump
Copy link
Member

jhump commented Aug 5, 2024

@jzbrooks, what version are you using? As of v0.5.0, you can set a property on the ProtocolClientConfig so that this dispatch happens automatically. The change was made here: #218.

@jzbrooks
Copy link
Contributor Author

jzbrooks commented Aug 6, 2024

Hey @jhump, that's exactly what I was looking for. I missed that note on the release. Thanks!

@jzbrooks jzbrooks closed this as completed Aug 6, 2024
@erawhctim
Copy link
Contributor

Any reason why this isn't enabled by default?

@jhump
Copy link
Member

jhump commented Aug 6, 2024

Any reason why this isn't enabled by default?

@erawhctim, at the time I added it, I was concerned about it being surprising behavior. Also it's not backwards compatible: if you have application code that did not configure this new property and explicitly dispatched to a particular context, the library under the hood would re-dispatch back to its default context, which may be surprising or problematic.

I'm certainly open to changing the default behavior though before we get to a v1.0.0 release. But I was thinking of making that change in the same release as other more significant (and backwards-incompatible) API changes, so that it's less likely to be overlooked when someone upgrades.

@erawhctim
Copy link
Contributor

very wise; that makes a lot of sense 👍 Setting that as the default for v1.0 sounds ideal.

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

No branches or pull requests

3 participants