Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wrapping libuv signal for use in Rust #9318

Closed
wants to merge 1 commit into from
Closed

wrapping libuv signal for use in Rust #9318

wants to merge 1 commit into from

Conversation

minhnhdo
Copy link
Contributor

This pull request aims to create a wrapper for libuv's signal handling for use in Rust. Although it is still in an early stage, early feedback is much welcomed.

@alexcrichton
Copy link
Member

This looks pretty good from a libuv-bindings perspective, but currently there is no safe method to use this. Right now you could register a handler, and then forget to unregister it, causing possible leaks or use-after-free situations.

I'm not entirely certain about the semantics of our &fn vs ~fn types, but it seems like perhaps there should be some safe bindings which have a destructor which un-registers the handler. Something along the lines of:

let handler = io::signal::Handler::new(SIGINT, || {
  println("look, I got a sigint!");
});
handler.install();
// handler is un-registered when it is destroyed.

Basically the main takeaway is that the main purpose of rt::io is to provide safe bindings to all the libuv functions. That's the main bulk of what still needs to be done for this pull.

@thestinger
Copy link
Contributor

An example of how to implement a useful/safe signal handling API is the Go standard library's signal package:

http://golang.org/pkg/os/signal/

The task can ask for a pipe on which to receive signals, and select on it. I expect you could also make a standalone signal object to select on.

@alexcrichton
Copy link
Member

I actually always thought that Go's signals were pretty slick. We're definitely going to want selection over a number of events at some point, and if signals could fit into that list, it would be awesome.

@minhnhdo
Copy link
Contributor Author

@alexcrichton uv_signal_init requires a uv_loop_t*. Could you tell me how to get one?
@thestinger I was using Go's documentation as a reference

@alexcrichton
Copy link
Member

YOu can get the uv_loop_t from the IoFactory or one of the related instances of that.

@minhnhdo
Copy link
Contributor Author

@alexcrichton, @thestinger could you give me an example of how selection would work?

@alexcrichton
Copy link
Member

I'm not entirely sure because I've never used them before, but it appears that you may be interested in src/libstd/select.rs

None => {
let chan = self.chan.clone();
let handler = ~do Handler::new(signum) |_, signum| {
chan.send(signum);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SharedChan doesn't like send()ing in a scheduler context (rtassert! triggered at src/libstd/rt/comm.rs:118). It seems that extern "C" fns are executed in the scheduler context (?). I have also tried to task::spawn a new task and send() in that task instead, but task::spawn requires that it be called in a green task context (rtassert! triggered at src/libstd/task/spawn.rs:561). Could you point me to a fix, @brson ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I came across this quite a bit in a branch i was spiking related to async IO (ended up getting dropped).

anyways.. the people i spoke to (I believe @toddaaro and/or @anasazi) indicated that that barrier to sending from a SchedTask was artificial..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olsonjeffery could you please confirm if I can delete the rtassert!?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, There actually are two things that need to happen:

  1. remove the rtassert!
  2. use send_deferred instead of send(that's what will actually cause a segfault if you send from a SchedTask

Can someone else verify this COA, though? @alexcrichton @brson ?

Copy link
Member

Choose a reason for hiding this comment

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

Sadly I know very little about the runtime, so I won't be much help here :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@olsonjeffery I did not remove the rtassert!. However, I switched to using SharedChan<T>::send_deferred() instead of SharedChan<T>::send() and the rtassert! isn't triggered any more.

@minhnhdo
Copy link
Contributor Author

r?

@alexcrichton
Copy link
Member

Signal handling is historically an incredibly tricky thing to get right. I would imaging that a lot of "the magic" is taken care for us by libuv, however.

It would be nice to have very explicit documentation about the exact behavior of the signal handling in order to clarify what exactly it means to receive a signal. Right now there's a few things in this pull I think are missing. Some of them may just require clarification, but they should all be thought about and considered.

  • What happens if two handlers are registered for the same signal?
  • There should certainly be tests.
  • What happens if a signal is received and no one is listening?
  • What happens if signals are received when previous ones have not been processed?

Those are the ones that I can think of off the top of my head, and I think that others would be alleviated with some more extensive documentation about the signal handling.

@thestinger
Copy link
Contributor

Especially if you register a handler for the same signal in different schedulers (threads) on a platform without signalfd.

@minhnhdo
Copy link
Contributor Author

@alexcrichton Whenever I do make check TESTNAME=src/libstd/rt/io/signal.rs, it always reports 0 test in test result. Could you tell me what I am doing wrong?

@alexcrichton
Copy link
Member

I'm not sure if TESTNAME works for unit tests, I normally run make check-stage1-std and then run the binary by hand with the string to filter on.

@huonw
Copy link
Member

huonw commented Sep 29, 2013

TESTNAME does work, but the --test test-runners filter by Rust name, not file path, so make check-stage1-std TESTNAME=rt::io::signal should work.

@minhnhdo
Copy link
Contributor Author

@huonw thanks. It works.
Is there any way to force the scheduler to send all data sent by send_deferred()?

@minhnhdo
Copy link
Contributor Author

minhnhdo commented Oct 2, 2013

need-info r?

@brson
Copy link
Contributor

brson commented Oct 16, 2013

It's a shame to let this keep sitting. Do we know what needs to happen to merge this?

@thestinger
Copy link
Contributor

@brson: I'm worried that LLVM isn't implementing this in a way that's safe with multiple libuv instances around, and haven't looked into it deep enough to know for sure.

@minhnhdo
Copy link
Contributor Author

@thestinger Could you elaborate? How can I check for this concern?

@alexcrichton
Copy link
Member

I think that this is still missing the documentation necessary at the top of the file in docblocks or associated with the various functions as well. Additionally, I'm still not sure what the answers to a couple of my above questions are, so would you be willing to write up some documentation explaining what's going on?

@minhnhdo
Copy link
Contributor Author

@alexcrichton I'm on it

Interrupt = 2i,
Break = 21i,
HangUp = 1i,
WindowSizeChange = 28i,
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some documentation for when each of these signals are generated? It would also be useful to mention their posix equivalents (if they exist)

@alexcrichton
Copy link
Member

I was looking into libuv's implementation, and they appear to handle most of my concerns, so I'm not too worried about the semantics of signal handling. It looks like everything here is essentially "what we'd want to do" in the sense that it's all the right plumbing. The more concerning part to me is that the libuv types and implementation are exposed directly to rt::io. This needs to be reorganized to follow the same layout as the rest of the implementations of I/O in rt::io.

That would involve making an Rtio type inside of rt::rtio and then adding a method to IoFactory. Inside of rt::uv::uvio you would then implement the Rtio type for some Uv type (which does much of this internally), and then everything is wrapped up and exposed through rt::io::signal in a wrapper which uses the IoFactory to create an object. Also, the return value of Listener::new should be Option<Listener> because there may be no local IoFactory.

I can touch up the documentation afterwards, I'm mostly concerned about the organization right now. Again, sorry this took awhile to get to, but now I feel like I'm up to speed with the runtime and feel comfortable doing an actual review :)

@minhnhdo
Copy link
Contributor Author

@alexcrichton thank you for this detailed review. I will fix it up and make a commit shortly.

@minhnhdo
Copy link
Contributor Author

r?

}
}

impl GenericPort<Signum> for Listener {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little unfamiliar with our concurrency primitives, but I imagine that doesn't expose the ability to select over the port receiving the signal, right? I think that it would be ok if port was a public field accessible by consumers of the api so the full functionality of the port could be leveraged.

@alexcrichton
Copy link
Member

This is looking good, nice work!

I had a few comments about how I think the EventLoop api should get reorganized, and I think that would draw a clearer boundary of what's responsible for what. That would push the Handler structure down into uvio (where I think it belongs), and a little more error handling would get exposed through the eventloop interface.

Again though, thanks for doing all this, it's sorely needed!

@minhnhdo
Copy link
Contributor Author

@alexcrichton thank you for your kind words. I have pushed the changes. r?

@@ -154,3 +159,7 @@ pub trait RtioPipe {
fn read(&mut self, buf: &mut [u8]) -> Result<uint, IoError>;
fn write(&mut self, buf: &[u8]) -> Result<(), IoError>;
}

pub trait RtioSignal {
fn signal(&mut self, signal: Signum, channel: SharedChan<Signum>) -> Result<(), IoError>;
Copy link
Member

Choose a reason for hiding this comment

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

Let's not expose this as a function on the trait, but rather leave it up to the implementation. It's a little odd to have a blank trait, but I think that it's proper for now.

@alexcrichton
Copy link
Member

Looking really good! Would you mind adding a test for the case I mentioned earlier, along with a test which generates an error when you attempt to listen on an invalid signal?

descriptive names
easier-to-use api
reorganize and document
@minhnhdo
Copy link
Contributor Author

@alexcrichton since there is an EventLoop trait already, I assume you meant to add to this one. Changes pushed. r?

@alexcrichton
Copy link
Member

Looks great to me! I'm going to merge this along with #9901 in order to reduce the number of merge conflicts it's going to cause, thanks again!

@alexcrichton
Copy link
Member

Thanks again for this! (still in the process of merging, but it'll be up on #9901 soon)

@minhnhdo
Copy link
Contributor Author

@alexcrichton thanks 👍

Jarcho pushed a commit to Jarcho/rust that referenced this pull request Aug 29, 2022
Fix if_let_mutex not checking Mutexes behind refs

Fixes rust-lang#9193

We can always peel references because we are looking for a method-call, for which autoderef applies.

---

changelog: [`if_let_mutex`]: detect calls to `Mutex::lock()` if mutex is behind a ref
changelog: [`if_let_mutex`]: Add labels to the two instances of the same Mutex that will deadlock
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.

7 participants