Skip to content

Add StreamReader.readinto() #85477

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

Closed
tontinton mannequin opened this issue Jul 15, 2020 · 11 comments
Closed

Add StreamReader.readinto() #85477

tontinton mannequin opened this issue Jul 15, 2020 · 11 comments
Labels
3.10 only security fixes topic-asyncio type-feature A feature request or enhancement

Comments

@tontinton
Copy link
Mannequin

tontinton mannequin commented Jul 15, 2020

BPO 41305
Nosy @njsmith, @asvetlov, @1st1, @aeros, @tontinton
PRs
  • bpo-41305: Add StreamReader.readinto() #21491
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = None
    created_at = <Date 2020-07-15.16:16:43.987>
    labels = ['type-feature', '3.10', 'expert-asyncio']
    title = 'Add StreamReader.readinto()'
    updated_at = <Date 2020-07-16.21:34:43.883>
    user = 'https://github.com/tontinton'

    bugs.python.org fields:

    activity = <Date 2020-07-16.21:34:43.883>
    actor = 'yselivanov'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['asyncio']
    creation = <Date 2020-07-15.16:16:43.987>
    creator = 'tontinton'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41305
    keywords = ['patch']
    message_count = 10.0
    messages = ['373702', '373714', '373719', '373720', '373732', '373750', '373765', '373770', '373772', '373773']
    nosy_count = 5.0
    nosy_names = ['njs', 'asvetlov', 'yselivanov', 'aeros', 'tontinton']
    pr_nums = ['21491']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue41305'
    versions = ['Python 3.10']

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 15, 2020

    Add a StreamReader.readinto(buf) function.

    Exactly like StreamReader.read() with *n* being equal to the length of buf.

    Instead of allocating a new buffer, copy the read buffer into buf.

    @tirkarthi tirkarthi added topic-asyncio 3.10 only security fixes type-feature A feature request or enhancement labels Jul 15, 2020
    @1st1
    Copy link
    Member

    1st1 commented Jul 15, 2020

    We don't want to extend StreamReader with new APIs as ultimately we plan to deprecate it. A new streams API is needed, perhaps inspired by Trio. Sadly, I'm -1 on this one.

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 15, 2020

    ok.

    Im interested in learning about the new api.
    Is it documented somewhere?

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 15, 2020

    Ah it's trio...

    @1st1
    Copy link
    Member

    1st1 commented Jul 15, 2020

    Im interested in learning about the new api.

    There are two problems with the current API:

    1. Reader and writer are separate objects, while they should be one.
    2. Writer.write is not a coroutine, although it should be one.

    There are other minor nits, but that's the crux of the problem. So we need a new streams design + a new set of APIs to work with it (and streams are in many places, like in the subprocess APIs).

    Trio was going to stabilize their own streaming API and we thought it would be great if our new API was compatible with it (not sure if they did stabilize it or not).

    I was going to lead the project myself (and still am) but dropped the ball and we missed 3.9 to do this. If you want to start working on this I'd be glad to assist with discussions & reviews.

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 16, 2020

    Ok actually that sounds really important, I am interested.

    But to begin doing something like this I need to know what's the general design.

    Is it simply combining stream reader and stream writer into a single object and changing the write() function to always wait the write (thus deprecating drain) and that's it?

    If there is not more to it I can probably do this pretty quickly, I mean it seems easy on the surface.

    If there is more to it then I would like a more thorough explanation. Maybe we should chat about this.

    @1st1
    Copy link
    Member

    1st1 commented Jul 16, 2020

    Is it simply combining stream reader and stream writer into a single object and changing the write() function to always wait the write (thus deprecating drain) and that's it?

    Pretty much. We might also rename a few APIs here and there, like "close()" should become an "async def aclose()"

    We also will likely want to define a few ABCs.

    Which brings me to the most important point: what we need it not coding it (yet), but rather drafting the actual proposal and posting it to https://discuss.python.org/c/async-sig/20. Once a formal proposal is there we can proceed with the implementation.

    It's a bit of a process to follow, but we have to do it this way.

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 16, 2020

    Which brings me to the most important point: what we need it not coding it (yet), but rather drafting the actual proposal and posting it to https://discuss.python.org/c/async-sig/20. Once a formal proposal is there we can proceed with the implementation.

    Posted: https://discuss.python.org/t/discussion-on-a-new-api-for-asyncio/4725

    By the way I know it's unrelated but I want a CR on #21446 I think it's also very important.

    @tontinton
    Copy link
    Mannequin Author

    tontinton mannequin commented Jul 16, 2020

    By the way if we will eventually combine StreamReader and StreamWriter won't this function (readinto) be useful then?

    Maybe we should consider adding it right now.

    Tell me your thoughts on this.

    @1st1
    Copy link
    Member

    1st1 commented Jul 16, 2020

    By the way if we will eventually combine StreamReader and StreamWriter won't this function (readinto) be useful then?

    Yes. But StreamReader and StreamWriter will stay for the foreseeable future for backwards compatibility pretty much frozen in time.

    So I'd like to start treating them as legacy APIs now.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @ezio-melotti ezio-melotti moved this to Todo in asyncio Jul 17, 2022
    @gvanrossum
    Copy link
    Member

    Okay, let's close this issue -- we're not going to add readinto(). I also left a note on the 2-year-old Discourse thread about the proposed new streams API, trying to revive that.

    @gvanrossum gvanrossum closed this as not planned Won't fix, can't repro, duplicate, stale Aug 28, 2022
    Repository owner moved this from Todo to Done in asyncio Aug 28, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes topic-asyncio type-feature A feature request or enhancement
    Projects
    Status: Done
    Development

    No branches or pull requests

    3 participants