-
Notifications
You must be signed in to change notification settings - Fork 278
Use the new Thread API to sandbox libsyntax use #164
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
Conversation
This allows RACER to continue processing even when libsyntax crashes. This is vital if we wish to use RACER as say, a library in the future
Is thread the only way to handle a panic ? |
@tafia: good point. But in that case, we'll have to manage the liveness of that thread (like, if that thread crashes, then we need to spin up a replacement thread). Plus, how would job control work? I'm not sure how you could use one thread to do this when (afaik) these procedures sometimes mutually recursively call each other. |
Yes
|
Can I suggest encapsulating this into a function? Otherwise that's a lot of boilerplate to copy-paste. Also it would be easier to toggle it off later. |
hm, but the problem with that is the fact that the way the error handling works is different for different functions. It could be modularized however, I don't really think that's necessary. Although, I could do this. I'm kind of debating on the pros versus cons of this. |
Hello! Thanks for the patch @bollu, and the conversation. I've been flip-flopping on this one. It makes racer half the speed again: before:
after:
However the issue of libsyntax crashing also affects some completions: racer is a quite liberal at the moment with what it passes to libsyntax. E.g. the following currently causes a crash: fn main() {
let v = Vec:: // <-- complete here
let b = 3;
} Whereas it used to complete fine and does again after this patch. (The reason is that while searching for "Vec", it matches the current line and attempts to parse it, but the statement overflows into the next line). Ideally libsyntax would change so that it doesn't panic any more. I suspect this will happen at some point since this is the direction rust is going in, although it may not be soon. In the meantime I'm thinking that we're going to need to apply this patch just to get racer working again in the presence of incomplete lines. Cheers, Phil |
Now that I think about it I haven't actually investigated the possibility that there could be a way to use libsyntax without panics. I'll investigate tonight if I have time. |
Oh duh, I think we're just aborting because of some code I cut-n-paste back in the early days! pub fn with_error_checking_parse<F, T>(s: String, f: F) -> T where F: Fn(&mut Parser) -> T {
let ps = new_parse_sess();
let mut p = string_to_parser(&ps, s);
let x = f(&mut p);
p.abort_if_errors(); // <<<<<<<<<<<<<<----------------------- HERE!
x
} Will remove this tonight and see what happens |
@phildawes: don't we need the |
I was testing this file against RACER to see what happens when there are errors. I removed the Here's the output:
The stack trace is here:
looking at the source code, it crashes here, with a call to So the |
Hello!, Thanks for the test case, I agree that a racer lib shouldn't panic. If we cannot get our libsyntax use to be panic-free then a better solution to wrapping every libsyntax call could be to wrap at a higher level (e.g. in the top-level function in the racer lib), for performance reasons. There's not much point avoiding the cost of spawning a process and then burning up to 10x that amount of time in thread creation and locking. Actually that leads me to a question: What's your main motivation for doing a sublime text FFI - is it performance or something else? Thanks, Phil |
We could also keep one thread for executing libsyntax and create new one each time the previous panics. |
hm, that seems reasonable-ish. But again, what happens if these are called recursively? If A calls B, and A was running on thread T, then what happens? to we spawn a new thread T1? how do we know when to spawn a new thread? |
it's a mixture of multiple things - FFI, more complex completion support, the need to not parse strings every time we pass through Racer, and just provide a better user interface. I'm planning on packaging RACER as a library next as part of my GSOC proposal. So this is the first step. |
Hi @bollu, I'm afraid the link to the GSOC proposal isn't working for me now. I think it was working earlier today. |
@phildawes: I updated the link - should work now - Here's the URL |
Hi @bollu, I had a look through the GSOC proposal. It would be cool to have you working on racer! A couple of things:
Racer in its current state doesn't have much (if anything) to cache between invocations - it does not compile whole files or crates into ast and it doesn't build a type graph. Do you intend to change racer so that it does this (e.g. by interfacing somehow with rustc and compiling crates globally into typed ast)? This would be significant work so it is worth elaborating on this. |
FYI: rust-lang/rust#23857 |
Closing as libsyntax has now been patched to remove panics and racer uses this. Further panics should be considered a bug and fixed in racer/libsyntax. Cheers! |
This allows RACER to continue processing even when libsyntax crashes.
This is vital if we wish to use RACER as say, a library in the future.
This fixes the issue of racer crashing
I realize that this commit sort of undoes this one where the threading code was removed, but from what I read on the issue tracker, that code was removed since Rust's behaviour changed and allowed
panic!(...)
to leak between threads.Since the leaking of
panic!(...)
does not happen, I think that this error handling must be reintroduced, regardless of performance cost (yes, this does have some performance penalty)