Is there an easy way to add tests for this?

Thanks for the review, test case added.

How does this relate toos.sendfile? Willos.sendfile andsocket.sendfile have divergent behaviour? OTOH, how is their behaviour now?

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

socket.sendfile is build uponos.sendfile, which will ensure all the data will be written. To get this, it may invokeos.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 start2, we will invoke it likes.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 befsize - offset, which is42 - 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 asblocksize.

Thensocket.sendfile will invokeos.sendfile(in_fd, out_fd, offset=2, count=42). So we give the syscallsendfile a valid range, which it's end is out of the file.

Modern systems which CPython supports withsendfile 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 theos.sendfile via Windows'sTransmitFile, and found thatTransmitFile don't accept the out of range count argument. (Meanwhile, it accepts theoffset 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 theTransmitFile have much more different behaviors to the Unix'ssendfile, the main block is thatTransmitFile does not respect theSO_SNDTIMEO option to make a timeout. Then I give up it, and just try to implement the high levelsocket.sendfile viaTransmitFile. 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!