Skip to content

Async wip#12

Open
cholcombe973 wants to merge 1 commit intomasterfrom
futures
Open

Async wip#12
cholcombe973 wants to merge 1 commit intomasterfrom
futures

Conversation

@cholcombe973
Copy link
Owner

@mzhong1 so I was playing around with the async functionality of libnfs and I'm wondering if this would be ok to use. This would require the library user to setup a function for nfs to call you later with. It would look like:

#[no_mangle]
pub unsafe extern "C" fn nfs_callback(result: i32, 
    *mut nfs_context, 
    *mut c_void, *mut c_void) {
    // do things
}

Not sure if that'll be ok or annoying to use.
@jsgf you mentioned you did this with oneshot. Could you provide an example maybe? I'm scratching my head about how that would work.

@cholcombe973 cholcombe973 requested a review from mzhong1 December 25, 2018 23:44
@jsgf
Copy link
Contributor

jsgf commented Dec 28, 2018

The basic idea is to create a oneshot, and put the tx end into the context structure which is passed to the callback. When the callback is called, it can extract the tx end of the oneshot and send the appropriate value. The rx end of the oneshot is then just the future everything else uses (perhaps with a little post-processing).

@cholcombe973 cholcombe973 force-pushed the futures branch 3 times, most recently from 1e6dc07 to 1f00947 Compare December 29, 2018 00:02
src/lib.rs Outdated
) where
F: FnOnce(Result<()>) -> Result<()> + 'static,
{
// unbox the callback function
Copy link

Choose a reason for hiding this comment

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

This is boxing, not unboxing ;)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Thanks I'll fix the comment :).

@cholcombe973
Copy link
Owner Author

So the odd thing is that when I call this mount_async function in my test code it never calls the callback function I setup. I've tried all different variants and I can't seem to get it working.

@jsgf
Copy link
Contributor

jsgf commented Dec 29, 2018

You need to get the fd from libnfs and plug it into Tokio's reactor so that it can respond to socket state changes.

In principle this is straightforward, you "just" need to:

  1. use nfs_get_fd to get the underlying socket fd,
  2. find out what events it wants with nfs_which_events
  3. register it with tokio by creating mio::unix::EventedFd / tokio::reactor::Registration
  4. wait until the registration returns an event (using poll_fn with Registration::poll_read_ready/poll_write_ready)
  5. call nfs_service with the appropriate events, which ends up calling the callbacks

In practice its awkward because libnfs doesn't consume everything from the socket on each call to nfs_service, which means that it assumes a poll-like level-triggered interface. However tokio/mio uses epoll in edge-triggered mode which will lead to a deadlock if you call it naively (since there might be still things to read from the socket, but you'll never get more events for them). Instead, you also need to call poll/nfs_service until there are not more events pending before returning to the tokio/mio event loop.

This is doubly awkward for nfs_mount_async because it ends up making numerous different sockets during mount (portmapper, mountd then nfs), all reusing the same fd number. This would work with poll, but it doesn't work with epoll because each fd needs to be explicitly unregistered/reregistered with the kernel API, and there's no libnfs callbacks/APIs to do that. In the end I did the mount synchronously (within a tokio_threadpool::blocking context) and then went async after that.

@cholcombe973
Copy link
Owner Author

cholcombe973 commented Dec 29, 2018

@jsgf thanks that actually explains a lot of what I'm seeing. libnfs thinks everything is fine but it's not calling the callback fn. I'll work on getting the polling working with Tokio. Looking closer at the async example libnfs has on the repo: https://github.com/sahlberg/libnfs/blob/master/examples/nfsclient-async.c#L259 you're right they setup a poll which I'm missing.
Yeah that mount part does sound rather annoying. I might do the same thing you did and just keep that sync.

@cholcombe973
Copy link
Owner Author

cholcombe973 commented Dec 30, 2018

@jsgf so this gets slightly further but crashes when I try to call poll_write_ready it also crashes on poll_read_ready so something is still wrong. This won't be the final code. I'm just trying to get something working haha.

mounting
libnfs:2 connection established
libnfs:2 connection established
libnfs:2 connection established
libnfs:2 connection established
nsf_get_fd: 5
nfs_which_events: 1
registered
thread 'main' panicked at 'no Task is currently running', libcore/option.rs:1008:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:476
   5: std::panicking::continue_panic_fmt
             at libstd/panicking.rs:390
   6: rust_begin_unwind
             at libstd/panicking.rs:325
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:77
   8: core::option::expect_failed
             at libcore/option.rs:1008
   9: <core::option::Option<T>>::expect
             at libcore/option.rs:322
  10: futures::task_impl::with
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-0.1.25/src/task_impl/mod.rs:43
  11: futures::task_impl::current
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-0.1.25/src/task_impl/mod.rs:115
  12: tokio_reactor::registration::Registration::poll_write_ready::{{closure}}
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-reactor-0.1.7/src/registration.rs:331
  13: core::ops::function::FnOnce::call_once
             at libcore/ops/function.rs:238
  14: tokio_reactor::registration::Inner::poll_ready
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-reactor-0.1.7/src/registration.rs:507
  15: tokio_reactor::registration::Registration::poll_ready
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-reactor-0.1.7/src/registration.rs:366
  16: tokio_reactor::registration::Registration::poll_write_ready
             at /home/chris/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-reactor-0.1.7/src/registration.rs:331
  17: libnfs::Nfs::setup_async
             at src/lib.rs:705
  18: basic::main
             at examples/basic.rs:14
  19: std::rt::lang_start::{{closure}}
             at libstd/rt.rs:74
  20: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  21: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
  22: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:392
             at libstd/rt.rs:58
  23: std::rt::lang_start
             at libstd/rt.rs:74
  24: main
  25: __libc_start_main
  26: _start

@cholcombe973
Copy link
Owner Author

Ah ok I see what's going on. I tried reversing the order of calls here and it started working and then seg faulted later. Reading the mio docs helped. I need to setup a bunch of calls and then call setup_async last to run the tasks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants