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

Make DEFAULT_LIMIT monkey patchable #4696

Closed
wants to merge 10 commits into from
Closed

Make DEFAULT_LIMIT monkey patchable #4696

wants to merge 10 commits into from

Conversation

spacemanspiff2007
Copy link

@spacemanspiff2007 spacemanspiff2007 commented Apr 18, 2020

What do these changes do?

Currently its not possible to change DEFAULT_LIMIT.
This change allows this:

import aiohttp
aiohttp.streams.DEFAULT_LIMIT = 10 * aiohttp.streams.DEFAULT_LIMIT

Are there changes in behavior for the user?

No

Related issue number

#4453

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

Currently its not possible to change DEFAULT_LIMIT.
This change allows this
```
import aiohttp
aiohttp.streams.DEFAULT_LIMIT = 10 * aiohttp.streams.DEFAULT_LIMIT
```
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Apr 18, 2020
@spacemanspiff2007
Copy link
Author

@webknjaz could you review this change, too? It would fix my application from crashing.

aiohttp/streams.py Outdated Show resolved Hide resolved
@spacemanspiff2007
Copy link
Author

I changed it as requested - it should be possible to merge this.

@spacemanspiff2007
Copy link
Author

Is there anything I can do to get this merged?

@robhaswell
Copy link

I am another user who would like to see this functionality.

@spacemanspiff2007
Copy link
Author

@webknjaz :
Are there any plans to merge this? This is a tiny change which would prevent my application from throwing errors.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't support this PR.
If you want to change the default buffer size -- please pass the desired value into client.get() or a similar call instead of monkey-patching. ClientSession can accept the session-default value as the argument constructor as well.

Sure, it requires a bigger pull request than initially proposed but I believe that explicit way is much better.

@spacemanspiff2007
Copy link
Author

Thank you for your reply. The crash happens when using aiohttp together with aiohttp-sse-client but I think if it's a possible Session arg this would be still fine.
A working example and finished PR would be here, it just has to be merged.

I am just looking for any (simple) way to change the buffer so my application doesn't go down with an exception

@asvetlov
Copy link
Member

@spacemanspiff2007 sorry again, I misled you and myself by writing controversial recommendations.

To don't bother you again I've prepared a fix myself as #5065
I hope it solves your needs.

@asvetlov
Copy link
Member

Closing the PR for the sake of #5056

@asvetlov asvetlov closed this Oct 17, 2020
@spacemanspiff2007
Copy link
Author

Thank you very much for your efforts and for the solution!

@robhaswell
Copy link

Sorry @asvetlov I must be being dumb, but I can't see how #5056 is related to the size of the line buffer?

@asvetlov
Copy link
Member

Sorry for the typo, the actual number is #5065 instead of #5056

@robhaswell
Copy link

Got it. Thanks for addressing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants