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

Deprecate stream.unread_data #3260

Closed
asvetlov opened this issue Sep 10, 2018 · 9 comments
Closed

Deprecate stream.unread_data #3260

asvetlov opened this issue Sep 10, 2018 · 9 comments
Labels

Comments

@asvetlov
Copy link
Member

stream.unread_data() is a leaking abstracton: it violates chunk boundaries at least.
Aso, it prevents some optimizations.

The call is used in multipart. We already have MultipartBodyReader._unread buffer, replacement stream.unread_data() wit appending to self._unread looks feasible.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 5, 2018

@kxepal I need your help
Can you spend a bit of your spare time on replacement stream.unread_data() with adding a read chunk into self._unread in multipart?

aiohttp 3.5 is going to be the latest 3.x release; I'd like to deprecate the method now and remove it entirely in aiohttp 4.0

@kxepal
Copy link
Member

kxepal commented Dec 5, 2018

Ok, will do that, tomorrow (6.12).

@asvetlov
Copy link
Member Author

asvetlov commented Dec 5, 2018

Thanks!

@kxepal
Copy link
Member

kxepal commented Dec 8, 2018

That seems wouldn't be too easy. We relay too much on that stream unread_data in different places and it serves a little bit different purpose than internal _unread - those one holds whole lines, while we have to unread arbitrary chunks of data. Mixing them breaks a lot.

I feel that I would have just to reimplement those stream unread logic on multipart side, what may be no very cool, but at least it will hold all the bits and not depend on foreign state.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 8, 2018

@kxepal thanks for the status update.

I need your opinion: can we live with #3439 for a while? I don't want to block the release on the issue

@kxepal
Copy link
Member

kxepal commented Dec 8, 2018

Yes, we can. For 4.0 we'll fix this issue.

@asvetlov
Copy link
Member Author

asvetlov commented Dec 8, 2018

@kxepal thanks!

@asvetlov
Copy link
Member Author

Fixed by #3439

@lock
Copy link

lock bot commented Dec 26, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Dec 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants