8000 gh-111347: Remove wrong assertion in test_sendfile by zcxsythenew · Pull Request #111377 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-111347: Remove wrong assertion in test_sendfile #111377

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 29, 2023
Merged

gh-111347: Remove wrong assertion in test_sendfile #111377

merged 4 commits into from
Oct 29, 2023

Conversation

zcxsythenew
Copy link
Contributor
@zcxsythenew zcxsythenew commented Oct 27, 2023 •
edited by gvanrossum
Loading

Closes gh-111347.

@bedevere-app bedevere-app bot mentioned this pull request Oct 27, 2023
Closed
@bedevere-app bedevere-app bot added awaiting review tests Tests in the Lib/test dir labels Oct 27, 2023
@gvanrossum
Copy link
Member
gvanrossum commented Oct 27, 2023 •
edited
Loading

From you bug report I understand that the tell() value is only unreliable with ProactorEventLoop. Maybe you can detect that combo and skip it?

@zcxsythenew
Copy link
Contributor Author

From you bug report I understand that the tell() value is only unreliable with ProactorEventLoop. Maybe you can detect that combo and skip it?

Let me have a try.

gvanrossum
gvanrossum approved these changes Oct 29, 2023
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I prefer to write the condition differently (and without \\) -- can you re-test on your platform?

gvanrossum
gvanrossum reviewed Oct 29, 2023
View reviewed changes
Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I just had a thought. I'd like @zooba to think about this briefly -- maybe the Proactor code should arrange for the file position to be correctly updated after calling transmitFile?

@gvanrossum gvanrossum requested review from zooba and removed request for 1st1 and asvetlov October 29, 2023 01:02
@zcxsythenew
Copy link
Contributor Author
zcxsythenew commented Oct 29, 2023 •
edited
Loading

I just had a thought. I'd like @zooba to think about this briefly -- maybe the Proactor code should arrange for the file position to be correctly updated after calling transmitFile?

I think it is a little bit difficult. Based on my experiment:

  • When the whole file is sent, the file position is at the end of the file.
  • When part of the file is sent, there is no guarantee where the file position will be, and transmitFile does not have any return value indicating this. (Usually less than where it should be.)

@gvanrossum
Copy link
Member

Okay, then let’s merge this!

@gvanrossum gvanrossum added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Oct 29, 2023
@gvanrossum gvanrossum merged commit fa35b9e into python:main Oct 29, 2023
@miss-islington-app
Copy link

Thanks @zcxsythenew for the PR, and @gvanrossum for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 29, 2023
@zcxsythenew @miss-islington
pythongh-111347: Remove wrong assertion in test_sendfile (pythonGH-11…
c241158 
…1377)

Windows is different.
(cherry picked from commit fa35b9e)

Co-authored-by: zcxsythenew <30565051+zcxsythenew@users.noreply.github.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 29, 2023
@zcxsythenew @miss-islington
pythongh-111347: Remove wrong assertion in test_sendfile (pythonGH-11…
30b4207 
…1377)

Windows is different.
(cherry picked from commit fa35b9e)

Co-authored-by: zcxsythenew <30565051+zcxsythenew@users.noreply.github.com>
@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

GH-111461 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Oct 29, 2023
@bedevere-app
Copy link
bedevere-app bot commented Oct 29, 2023

GH-111462 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Oct 29, 2023
willingc pushed a commit that referenced this pull request Oct 29, 2023
@miss-islington @zcxsythenew
[3.12] gh-111347: Remove wrong assertion in test_sendfile (GH-111377) (…
0681b4c 
…#111461)

gh-111347: Remove wrong assertion in test_sendfile (GH-111377)

Windows is different.
(cherry picked from commit fa35b9e)

Co-authored-by: zcxsythenew <30565051+zcxsythenew@users.noreply.github.com>
willingc pushed a commit that referenced this pull request Oct 31, 2023
@miss-islington @zcxsythenew
[3.11] gh-111347: Remove wrong assertion in test_sendfile (GH-111377) (…
a9d5966 
…#111462)

gh-111347: Remove wrong assertion in test_sendfile (GH-111377)

Windows is different.
(cherry picked from commit fa35b9e)

Co-authored-by: zcxsythenew <30565051+zcxsythenew@users.noreply.github.com>
FullteaR pushed a commit to FullteaR/cpython that referenced this pull request Nov 3, 2023
@zcxsythenew @FullteaR
pythongh-111347: Remove wrong assertion in test_sendfile (python#111377)
b369a0b 
Windows is different.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@zcxsythenew @aisk
pythongh-111347: Remove wrong assertion in test_sendfile (python#111377)
20a8924 
Windows is different.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@zcxsythenew @Glyphack
pythongh-111347: Remove wrong assertion in test_sendfile (python#111377)
03222b3 
Windows is different.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@willingc willingc Awaiting requested review from willingc willingc is a code owner

@zooba zooba Awaiting requested review from zooba

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

test_asyncio: test_sendfile failed on ReFS, Windows 11 Dev Drive
3 participants
@zcxsythenew @gvanrossum @willingc
0