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

Can NetworkResponse be Parcelable? #46

Closed
alexchau-google opened this issue Jun 19, 2017 · 4 comments
Closed

Can NetworkResponse be Parcelable? #46

alexchau-google opened this issue Jun 19, 2017 · 4 comments
Milestone

Comments

@alexchau-google
Copy link

Currently VolelyError and subclasses has @SuppressWarnings("serial") because NetworkResponse is not serializable. Indeed, it is very easy to make it serialziable, as shown in this clone project: mcxiaoke/android-volley@4012176
This will help developer to pass around VolleyError cross process or through Intent (Extras support Serializable).

@alexchau-google
Copy link
Author

@jpd236
Copy link
Collaborator

jpd236 commented Jun 19, 2017

Serializable is generally not a wise idea on Android. It's slow and generates a ton of garbage:

https://stackoverflow.com/questions/3611843/is-using-serializable-in-android-bad

Making it Parcelable might be reasonable, but I'd need to think through whether that might have bad implications on potential existing subclasses of NetworkResponse, which is not final.

I would suggest just making your own Parcelable helper classes as you see fit in the meantime.

@jpd236 jpd236 changed the title NetworkResponse should be serializable Can NetworkResponse be Parcelable? Jun 19, 2017
@alexchau-google
Copy link
Author

Got it. Parcelable also works, our only use case is for passing around as part of Intent.

@jpd236 jpd236 added this to the 1.1.0 milestone Sep 21, 2017
@jpd236
Copy link
Collaborator

jpd236 commented Oct 12, 2017

On reflection, I think it would be best to avoid doing this in NetworkResponse itself. While in general Volley is intended to be used for small response bodies, NetworkResponse data can be somewhat large given that it contains the entire response body as a byte array and the list of all response header keys/values.

Binder is intended for transmission of very small values; it will crash your app if you try to send anything larger than even 1 MB over it (and that limit is shared over a whole process so the effective limit can be even less than this). This kind of crashing can be a bit tricky, e.g. you might be able to pass the data in an Intent and have it work, but then crash when you rotate the screen which triggers a parcel/unparcel pass.

While it can be reasonable to send this data directly if you know your requests/responses are small, I don't think we should make it easy to do that in Volley without thinking through these ramifications. Instead, I think it should be up to client apps to decide carefully what they should/shouldn't send over the parcel boundary by extracting needed information out of the NetworkResponse and only sending that. Ideally only the specific info that needs to be transferred over binder can be, or if larger transfers are needed then a more scalable approach like a ContentProvider should be used.

@jpd236 jpd236 closed this as completed Oct 12, 2017
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

2 participants