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

void #3250

Closed
ghost opened this issue Dec 29, 2023 · 7 comments
Closed

void #3250

ghost opened this issue Dec 29, 2023 · 7 comments

Comments

@ghost
Copy link

ghost commented Dec 29, 2023

No description provided.

@sethmlarson
Copy link
Member

I don't think we should be configuring this at the level of an HTTP response, the response is an IO object so its behavior should be dictated by the consuming user by calling either .read() or .read1().

@sethmlarson
Copy link
Member

Does calling read versus read1 matter for preloaded content? We have to block until everything is downloaded anyways, there's no "opportunistic" reads/streaming we can do in that case.

@ghost ghost changed the title HTTPResponse: introduce read1 parameter to further methods HTTPResponse.read1(): Fix, Revise and Document Dec 31, 2023
@sethmlarson
Copy link
Member

sethmlarson commented Jan 1, 2024

Brought this up in Discord too, copying here. I think HTTPResponse.read1()'s API contract is incompatible with decode_content since we can't uphold the IO stream contract of always returning some bytes if amt > 0 and the stream isn't EOL. For HTTPResponse.read() we can uphold the contract because we're allowed to issue multiple underlying read() calls, read1() requires us to only issue one syscall though so we can't guarantee that we'll receive enough data in order to emit decompressed data.

@pquentin
Copy link
Member

pquentin commented Jan 2, 2024

It's not necessarily flawed by design, but it's yet another issue. We could either forbid decode_content in read1() or add a warning in the documentation. (For context, we worked hard in #2798 and other places to fix this for read().)

@illia-v
Copy link
Member

illia-v commented Jan 3, 2024

The fact that read1 can make more than one call on the raw stream is a good point. As noted before, this happens and is required only when decode_content=True. I'd vote for documenting this properly rather than dropping decode_content for read1.
One of possible use cases for read1 is streaming #3216, having no possibility to decode the data is a deal breaker for it.

@illia-v
Copy link
Member

illia-v commented Jan 7, 2024

The fact that read1 can make more than one call on the raw stream

*can't?

The current version of urllib3's HTTPResponse can.

his streaming #3216, having the possibility to decode the data

*not having?

Yes, "not having".

I believe that we can all agree on one thing:

That further work is needed to "Validate/Specify, Revise and Document" read1, and fix any upcoming issues.

Revising code is a strength of me (especially when i'm not very familiar with the code.

Would just need you to apply some bounty-budget to the issues.

It is unclear what you see as the expected result of #3257 at the moment.
Thank you for bringing up the discussion on the compromise between having decode_content for read1 and following the contract of making up to one call on the raw stream. Since the functionality has not been to any release yet, we can make a choice of either dropping the parameter or documenting that read1 can make more than one call. The decision should be collective though.

@sethmlarson
Copy link
Member

sethmlarson commented Jan 8, 2024

@abebeos Thank you for your interest in contributing to urllib3. I wanted to provide some context into our project and feedback on your approach. I hope that my comment here won't discourage you and instead will help you be more effective towards your goals.

Bounties are only assigned to tasks which have consensus from maintainers and are clearly defined solutions. Tasks which don't have consensus or aren't clearly defined (like this one) won't have bounties assigned until they are clear and agreed upon. It's okay to ask for an issue to be assigned a bounty once it meets this threshold.

Speaking for myself, your approach of opening many issues and PRs all mentioning bounties does not jive well with me, and that may not have been your intent but it is the effect it's had on me.

If your goal is to be paid a bounty (something that we want to do, we want to pay you!) you should be working on gaining consensus from maintainers by feeding our context or making concrete and concise proposals. Making tons of comments, linking to other places, marking things as outdated, proposing PRs and then closing them later, all of that gets in the way of maintainers understanding the problem and your proposed solution. We only have so many minutes in a day to spend on open source, so clarity and conciseness count.

I also wanted to note from some of your PRs there is a sense of "merge if it's not incorrect, we can figure it out later". We don't take that approach with this project, doing something for no reason or an unclear reason is a negative outcome even if on the surface the PR is a no-op in terms of value to the project. We value intentionality over getting things done quickly, because one day many years in the future someone (unlikely to be you or me!) will have to know why we made certain changes (and we don't know which ones will matter!)

Again, I hope you understand that this feedback comes from a place of wanting to see you succeed and to ultimately pay you to contribute to our project. Thanks again!

@ghost ghost changed the title HTTPResponse.read1(): Fix, Revise and Document void Jan 9, 2024
@ghost ghost closed this as completed Jan 9, 2024
This issue was closed.
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