Add socket sendfile function #2311

rodrigocam · 2020-10-26T17:56:34Z

Add socket function that sends all content of a file through the socket.

coolreader18 · 2020-10-27T03:32:02Z

I'm so sorry, I should've checked before but this isn't actually how the sendfile method is implemented. It's implemented in the Python socket.py module, and it calls os.sendfile(), which is a low-level function. If you'd like to take a shot at that you can try it (cpython version, linux signature, macos signature) but otherwise this isn't really correct; hopefully you got a bit familiar with the codebase at least. Sorry again 😣

rodrigocam · 2020-10-27T14:06:10Z

Oh, no problem. I will try to implement using low level functions. I saw that if the OS doesn't have sendfile it use send like I did, this will occur on Windows. So in our implementation, we will have a conditional compilation that will choose linux signature or mac signature to os.sendfile and in socket.sendfile we will use it right? There is any example of doing this conditional compilation?

coolreader18 · 2020-10-27T17:23:56Z

So in our implementation, we will have a conditional compilation that will choose linux signature or mac signature to os.sendfile and in socket.sendfile we will use it right?

Yup, pretty much. We don't even need to edit socket.py since it already checks that os.sendfile exists and uses it to provide socket.sendfile if it does:

RustPython/Lib/socket.py

Line 343 in 3c9ad23

if hasattr(os, 'sendfile'):

For examples, you can look at other functions in os.rs, e.g. getgroups is implemented separately in macos than it is on linux. For sendfile, you'd probably want something like this:

#[cfg(any(target_os = "linux", target_os = "otheros..."))]
#[pyfunction]
fn sendfile(out_fd: i32, in_fd: i32, offset: ...) -> PyResult<...> {
   ...
}

#[cfg(any(target_os = "macos", target_os = "otheros..."))]
#[pyfunction]
fn sendfile(out_fd: i32, in_fd: i32, ..., headers: PyObjectRef, trailers: PyObjectRef, flags: i32) -> PyResult<...> {
    ...
}

You probably want to use the nix crate's sendfile wrappers, linux and macos; that would mean you wouldn't have to deal with the raw libc APIs yourself.

rodrigocam · 2020-10-27T20:14:24Z

Nice! I implemented the os.sendfile function and added little tests in python, can you check?

coolreader18 · 2020-10-27T21:07:10Z

extra_tests/snippets/stdlib_os.py

+src_fd = os.open('README.md', os.O_RDONLY)
+dest_fd = os.open('destination.md', os.O_RDWR | os.O_CREAT)
+src_len = os.stat('README.md').st_size
+
+bytes_sent = os.sendfile(dest_fd, src_fd, 0, src_len)
+assert src_len == bytes_sent
+
+os.lseek(dest_fd, 0, 0)
+assert os.read(src_fd, src_len) == os.read(dest_fd, bytes_sent)
+os.close(src_fd)
+os.close(dest_fd)
+


You probably want to put this in a if hasattr(os, "sendfile"): block

coolreader18 · 2020-10-27T21:14:12Z

vm/src/stdlib/os.rs

+            headers.into_option(),
+            trailers.into_option(),


You have to process these into an array of buffers (&[&[u8]]) first -- something like

let headers = match headers.into_option() { Some(x) => Some(vm.extract_elements::<PyBytesLike>(&x)?), None => None, }; let headers = headers.as_ref().map(|v| v.iter().map(|b| b.borrow_value()).collect::<Vec<_>>()); let headers = headers.as_ref().map(|v| v.iter().map(|borrowed| &**borrowed).collect::<Vec<_>>()); let headers = headers.as_deref();

And then the same thing for trailers. That's really inefficient, what with creating like 3 different vecs, but I think I could submit a PR to nix to allow sendfile to accept any IntoIterator over AsRef<[u8]>, and then we could make this more efficient later.

So basically with those lines we are transforming PyObject into bytes? I put those lines and it worked, now the macos tests give me this error:

359 | pub(crate) fn sendfile( | ^^^^^^^^ the trait `function::sealed::PyNativeFuncInternal<_, _, _>` is not implemented for `for<'r> fn(i32, i32, i64, i64, function::OptionalArg, function::OptionalArg, function::OptionalArg<i64>, &'r vm::VirtualMachine) -> std::result::Result<pyobjectrc::PyObjectRc, pyobject::PyRef<exceptions::PyBaseException>> {stdlib::os::_os::sendfile}` |

Oh, to fix that you can add another macro call (or 2?) after line 642 in vm/src/function.rs

And yep, PyBytesLike is anything that implements the buffer protocol (e.g. bytes, bytearray, memoryview), PyBytesLike.borrow_value() gets a read lock to the data, and then the &** dereferences it to a &[u8].

Done! Now that os module has sendfile function, a python test is broken because it uses a create_file function that tries to open the file in xb mode, and there's no implementation for this mode.

coolreader18 · 2020-10-28T03:16:16Z

I added a commit to skip the test that's failing because of an unrelated issue with os.open, I think otherwise this is good to merge! (if CI doesn't fail for some reason)

rodrigocam · 2020-10-28T03:38:23Z

Thanks! I loved to contribute to this project, what do you think are the next steps to keeping contributing to the project?

youknowone

test_shutil also fails for now

youknowone · 2020-10-28T14:10:54Z

vm/src/stdlib/os.rs

+        count: i64,
+        headers: OptionalArg<PyObjectRef>,
+        trailers: OptionalArg<PyObjectRef>,
+        flags: OptionalArg<i32>,


Adding _ prefix like _flags will remove this warning

coolreader18 · 2020-10-28T17:18:39Z

Looks great, thanks for contributing! re: continue contributing, one way you can do it is to find tests that are expectedFailures/skipped and fix them, or else just try to do stuff in rustpython that you might do usually in python and see if it works (heads up, anything that uses numpy isn't going to happen for a while, since we don't have a c-api)

youknowone · 2020-10-29T04:40:37Z

This article maybe helpful for test work: https://rustpython.github.io/guideline/2020/04/04/how-to-contribute-by-cpython-unittest.html

Add socket sendfile function

00920ba

rodrigocam mentioned this pull request Oct 26, 2020

Extend the socket module #1176

Closed

Add os sendfile function and python test

a656c51

rodrigocam force-pushed the master branch 2 times, most recently from f4b9bd6 to ca054c0 Compare October 27, 2020 20:35

Remove socket sendfile wrong implementation

01e6f73

rodrigocam force-pushed the master branch from ca054c0 to 01e6f73 Compare October 27, 2020 21:03

coolreader18 reviewed Oct 27, 2020

View reviewed changes

Add guard clause to check if os has sendfile function

a8d579d

rodrigocam force-pushed the master branch 3 times, most recently from 7707f5a to eda4f73 Compare October 27, 2020 22:52

Process headers and trailers as array of byte buffers

a64ecbf

rodrigocam force-pushed the master branch 2 times, most recently from 5c879b0 to 84227bb Compare October 28, 2020 00:57

Add macro call to allow function up to seven parameters

6de2dc6

rodrigocam force-pushed the master branch from 84227bb to 6de2dc6 Compare October 28, 2020 02:39

Skip unrelated test in test_os

7d45672

youknowone reviewed Oct 28, 2020

View reviewed changes

Skip shutil test

ad3922a

coolreader18 approved these changes Oct 28, 2020

View reviewed changes

coolreader18 merged commit 525ff49 into RustPython:master Oct 28, 2020

Add socket sendfile function #2311

Add socket sendfile function #2311

Uh oh!

Conversation

rodrigocam commented Oct 26, 2020

Uh oh!

coolreader18 commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigocam commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coolreader18 commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigocam commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coolreader18 Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

rodrigocam Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

coolreader18 Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

rodrigocam Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolreader18 Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

coolreader18 Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigocam Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coolreader18 commented Oct 28, 2020

Uh oh!

rodrigocam commented Oct 28, 2020

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

youknowone Oct 28, 2020

Choose a reason for hiding this comment

Uh oh!

coolreader18 commented Oct 28, 2020

Uh oh!

youknowone commented Oct 29, 2020

Uh oh!

Uh oh!

coolreader18 commented Oct 27, 2020 •

edited

Loading

rodrigocam commented Oct 27, 2020 •

edited

Loading

coolreader18 commented Oct 27, 2020 •

edited

Loading

rodrigocam commented Oct 27, 2020 •

edited

Loading

rodrigocam Oct 27, 2020 •

edited

Loading

coolreader18 Oct 28, 2020 •

edited

Loading

rodrigocam Oct 28, 2020 •

edited

Loading