bpo-41687: Fix sendfile implementation to work with Solaris #22040
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kulikjak
commented
Sep 1, 2020
| @@ -446,6 +446,8 @@ def test_sendfile_ssl_close_peer_after_receiving(self): | |||
| self.assertEqual(srv_proto.data, self.DATA) | |||
| self.assertEqual(self.file.tell(), len(self.DATA)) | |||
|
|
|||
| @unittest.skipIf(sys.platform.startswith('sunos'), | |||
| "Doesn't work on Solaris") | |||
| def test_sendfile_close_peer_in_the_middle_of_receiving(self): | |||
There was a problem hiding this comment.
This single test fails with "AssertionError: ConnectionError not raised" and I am not sure why. With this patch, sendfile is not called if an offset is too high, which is slightly different from other platforms where it is called and sends nothing; but I don't think that causes this issue.
It seems to me that the entire message is sent and close_after doesn't work as expected. But I don't see into asyncio internals that much, so it's just a guess.
It was failing before as well, so it doesn't seem to be a regression. If you have any idea why that might be the case, how to test it further, or how to fix that, I will gladly do so (I don't really want to skip it).
There was a problem hiding this comment.
It was failing before as well, so it doesn't seem to be a regression. If you have any idea why that might be the case, how to test it further, or how to fix that, I will gladly do so (I don't really want to skip it).
Yeah, I don't like skipping it either. Unfortunately I won't be able to assist with the debugging, but if you have time please try to figure this out.
There was a problem hiding this comment.
I am getting closer. It looks like no matter the BUF_SIZE, sendfile always sends at least 131072 bytes, so it sends everything before transport is closed and hence no ConnectionError is raised. If I understand it correctly, the intended behavior is to limit sendfile to send at maximum BUF_SIZE bytes, meaning that transport is closed long before everything is sent (correct me if I'm wrong). I found out that the test passes if DATA size is doubled.
My guess is that this might be a difference in how Solaris handles those buffer limits; I will keep digging.
There was a problem hiding this comment.
I believe I found the culprit - the reason is that as per documentation: "At present, lowering SO_RCVBUF on a TCP connection after it has been established has no effect". This is on Oracle Solaris, but the bug is old enough that it should be present in all OpenSolaris forks unless they fixed it themselves.
I don't know If you'd prefer having it skipped here for all Solarises or if we should skip it internally since it is an OS bug, not a difference (I think that both are valid).
I checked Illumos source code and I believe they are also affected by this SO_RCVBUF related bug (but I might have missed their fix somewhere where I was not looking and I cannot verify that on Illumos machine).
There was a problem hiding this comment.
Let's skip the test on Solaris and add an elaborate comment why, basically summarizing
"At present, lowering SO_RCVBUF on a TCP connection after it has been established has no effect". This is on Oracle Solaris, but the bug is old enough that it should be present in all OpenSolaris forks unless they fixed it themselves.
1st1
reviewed
Sep 1, 2020
1st1
approved these changes
Sep 5, 2020
|
@1st1: Please replace |
|
Thank you, @kulikjak! |
|
|
You're welcome, but I just realized that this fix has broken error handling. I opened another PR that fixes that #22128; sorry for that. After that is fixed, can this be backported to at least 3.9? |
IDK, @ambv what do you think? |
miss-islington
pushed a commit
that referenced
this pull request
Sep 9, 2020
shihai1991
added a commit
to shihai1991/cpython
that referenced
this pull request
Sep 10, 2020
* origin/master: (1373 commits) bpo-1635741: Port mashal module to multi-phase init (python#22149) bpo-1635741: Port _string module to multi-phase init (pythonGH-22148) bpo-1635741: Convert _sha256 types to heap types (pythonGH-22134) bpo-1635741: Port the termios to multi-phase init (PEP 489) (pythonGH-22139) bpo-41732: add iterator to memoryview (pythonGH-22119) bpo-40744: Drop support for SQLite pre 3.7.3 (pythonGH-20909) bpo-41316: Make tarfile follow specs for FNAME (pythonGH-21511) bpo-41720: Add "return NotImplemented" in turtle.Vec2D.__rmul__(). (pythonGH-22092) bpo-1635741 port _curses_panel to multi-phase init (PEP 489) (pythonGH-21986) bpo-1635741: Port _overlapped module to multi-phase init (pythonGH-22051) bpo-1635741: Port _opcode module to multi-phase init (PEP 489) (pythonGH-22050) bpo-1635741 port zlib module to multi-phase init (pythonGH-21995) [doc] Add link to Generic in typing (pythonGH-22125) bpo-41513: Expand comments and add references for a better understanding (pythonGH-22123) bpo-1635741: Port _sha1, _sha512, _md5 to multiphase init (pythonGH-21818) closes bpo-41723: Fix an error in the py_compile documentation. (pythonGH-22110) [doc] Fix padding in some typing definitions (pythonGH-22114) Fix documented Python version for venv --upgrade-deps (pythonGH-22113) bpo-40318: Migrate to SQLite3 trace v2 API (pythonGH-19581) bpo-41687: Fix sendfile implementation to work with Solaris (python#22040) ...
|
Sorry @kulikjak and @1st1, I had trouble checking out the |
ambv
pushed a commit
to ambv/cpython
that referenced
this pull request
Sep 16, 2020
…thonGH-22040) (cherry picked from commit 8c0be6f) Co-authored-by: Jakub Kulík <Kulikjak@gmail.com>
|
GH-22273 is a backport of this pull request to the 3.9 branch. |
ambv
added a commit
that referenced
this pull request
Sep 16, 2020
miss-islington
pushed a commit
to miss-islington/cpython
that referenced
this pull request
Sep 16, 2020
…honGH-22128) I just realized that my recent PR with sendfile on Solaris ([PR 22040](python#22040)) has broken error handling. Sorry for that, this simple followup fixes that. Automerge-Triggered-By: @1st1 (cherry picked from commit fa8c9e7) Co-authored-by: Jakub Kulík <Kulikjak@gmail.com>
xzy3
pushed a commit
to xzy3/cpython
that referenced
this pull request
Oct 18, 2020
xzy3
pushed a commit
to xzy3/cpython
that referenced
this pull request
Oct 18, 2020
…honGH-22128) I just realized that my recent PR with sendfile on Solaris ([PR 22040](python#22040)) has broken error handling. Sorry for that, this simple followup fixes that. Automerge-Triggered-By: @1st1
|
This has been backported to 3.9 but not to 3.8, was it intentional? I can create a backport PR if it's ok. |
We don't backport compatibility fixes like this too far. It was backported to 3.9 because it wasn't released yet. |
jessecomeau87
pushed a commit
to jessecomeau87/Python
that referenced
this pull request
May 20, 2024
…22128) I just realized that my recent PR with sendfile on Solaris ([PR 22040](python/cpython#22040)) has broken error handling. Sorry for that, this simple followup fixes that. Automerge-Triggered-By: @1st1
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sendfile on Solaris raises EINVAL if offset is equal or bigger than the size of the file. This is different from Linux, where similar sendfile call returns 0, which is used in an ad-hoc fashion to indicate EOF. This patches in additional offset check to make the implementation Solaris compatible.
https://bugs.python.org/issue41687