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

Work around line length limit of asyncio.StreamReader #53

Merged
merged 5 commits into from
Apr 14, 2023

Conversation

felixfontein
Copy link
Collaborator

Fixes the problem mentioned in ansible-community/antsibull-docs#122 (comment).

@gotmax23
Copy link
Collaborator

FWIW, subprocess_tee, which somewhat inspired this implementation, sets

STREAM_LIMIT = 2**23  # 8MB instead of default 64kb, override it if you need

https://github.com/pycontribs/subprocess-tee/blob/7631e695a0d7fbc7c9622636aa298e4091894016/src/subprocess_tee/__init__.py#L254/11/23

Setting some limit means we don't have to complicate this code, and also, I don't think the logger can't handle unlimitted line lengths. See #52 (comment).

@felixfontein
Copy link
Collaborator Author

FWIW, subprocess_tee, which somewhat inspired this implementation, sets

STREAM_LIMIT = 2**23  # 8MB instead of default 64kb, override it if you need

https://github.com/pycontribs/subprocess-tee/blob/7631e695a0d7fbc7c9622636aa298e4091894016/src/subprocess_tee/__init__.py#L254/11/23

That sounds like a huge hack, and something we should absolutely not do.

Setting some limit means we don't have to complicate this code, and also, I don't think the logger can't handle unlimitted line lengths. See #52 (comment).

No, but I don't think we should decide what the logger can handle and what not. We should pass on what the program returns, no matter what it is (as long as it fits in memory).

Copy link
Collaborator

@gotmax23 gotmax23 left a comment

Choose a reason for hiding this comment

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

Alright, I'm convinced. This looks good to me, but I have one question. Thanks!

@felixfontein felixfontein merged commit d910be2 into ansible-community:main Apr 14, 2023
@felixfontein felixfontein deleted the limit branch April 14, 2023 04:35
@felixfontein
Copy link
Collaborator Author

@gotmax23 thanks for reviewing this!

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

Successfully merging this pull request may close these issues.

2 participants