Is there an easy way to add tests for this?

Thanks for the review, test case added.

How does this relate to os.sendfile? Will os.sendfile and socket.sendfile have divergent behaviour? OTOH, how is their behaviour now?

os.sendfile is just a wrapper function which almost directly calls the syscall function sendfile. The sendfile is just like the write, it may not send all the data you give to it.

socket.sendfile is build upon os.sendfile, which will ensure all the data will be written. To get this, it may invoke os.sendfile multiple times.

The problem this PR want to resolve is that, if we have a file with size 42, and want to send it to a socket from the start 2, we will invoke it like s.sendfile(f, offset=2, count=None).

Because we do not specify the count parameter, socket.sendfile will assume that we want send the whole file from the offset. So the count should be fsize - offset, which is 42 - 2 = 40. And the count is << 1G (max block size), so we will use count as block size.

The current codes in main branch missed the - offset, so it will use 42 as blocksize.

Then socket.sendfile will invoke os.sendfile(in_fd, out_fd, offset=2, count=42). So we give the syscall sendfile a valid range, which it's end is out of the file.

Modern systems which CPython supports with sendfile feature will accept it, and only returns the actually sent bytes (is 40 in this case). So nothing goes wrong here.

But I've tried to implement the os.sendfile via Windows's TransmitFile, and found that TransmitFile don't accept the out of range count argument. (Meanwhile, it accepts the offset is out of range to the file).

So I think this is a bug and we should fix it.

But, the is not the end of this story. I found that the TransmitFile have much more different behaviors to the Unix's sendfile, the main block is that TransmitFile does not respect the SO_SNDTIMEO option to make a timeout. Then I give up it, and just try to implement the high level socket.sendfile via TransmitFile. This PR is opened as #112337.

And then the change in this PR is not a must to have. But I still feels like it's a bug, so we should fix it 😂.

Thanks, @aisk, makes sense to me! cc. @serhiy-storchaka if he want's to take a look. I'm aiming for landing this in a couple of days.

Could you try to compose a short NEWS entry for this? I'm unsure if we really need one; this feels like an implementation detail.

See also #114077 and #114079.

There are some discussions on the original issue #112341. I think this should not be treated as a bug, so I'm closing this. If anyone wants to discuss it further, we can reopen it. Still appreciate the review!