Make netext httpext Response's Request a pointer #2271
Conversation
Making the Request attribute of the Response struct a pointer will avoid making copies of said Request objects (as they can be rather big) everytime we instantiate a Response. This is in line with the Go standard library.
There was a problem hiding this comment.
This mostly exists for response.request.body but is it useful? Could be the same think Go is doing an option?
// Request's Body is nil (having already been consumed).
from:
// Request is the request that was sent to obtain this Response.
// Request's Body is nil (having already been consumed).
// This is only populated for Client requests.
Request *Request
|
@yorugac This tiny refactor is the result of both reading about #1931 and discussions I've had with @mstoykov. It doesn't claim in any way to be directly related/fixing #1931. Hence me only refering it to said issue, and not marking it as closing, to indicate it popped out in the same context. My bad if there was any confusion on that front 😸. I'm not sure I see correlation with #2226, or at least not ones that keep from merging this. Do you have anything specific in mind? @codebien I am not entirely sure I understood your point, could you elaborate? Regardless of the larger context, I only meant to post a tiny improvement here, in the spirit of the broken window theory: not necessarily to make a big structural change. If there are larger implications, I would prefer to open and address them as a separate issue if that's okay with you 😸 PS: Sourcegraph shows the Response's request is actually used in a few places accross the standard library. Cheers 🐻 |
|
@oleiade I simply meant if there can be a conflict between this PR and #2226, as they're changing the same code: I didn't really look at the #2226 since I'm not among the reviewers there so wished to clarify that point. Anyway, I suppose @na--'s approval can be interpreted as an indirect answer being "no conflicts" 🙂 |
Rather "I need to rebase anyway, so I can fix any conflicts that arise" 😅 |
|
I don't really this helps all that much (and now I understand what you meant "doesn't use a pointer" when we're talking about this). The problem I was talking about with you about "the response having a copy of the request"(I think that is what I said) isn't so much it's a copy it's that it has the whole request with all of its different fields and in particular having the Body of the request as a string. I do think this is better though so 👍 ;) p.s. @codebien I think you are conflating http.Request (from the standard library) and httpext.Request (which is discussed here) - the second doesn't even have a copy of the first (which might be an interesting change 🤔 ) |
A tiny PR making the
Requestattribute ofnetext/httpext/Responsestruct a pointer. This should avoid copies of said Request objects (as they can be rather big) to be made every time we instantiate a Response. This is in line with Go's standard library.This is a tiny change, which, as far as I can tell, does not break tests. I've also verified that it does not break the JSON serialization of requests (pointers can be tricksters on that front).
ref #1931