Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

bollu
Copy link
Contributor

@bollu bollu commented Feb 25, 2015

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)

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
@tafia
Copy link
Contributor

tafia commented Feb 25, 2015

Is thread the only way to handle a panic ?
Can't we have a unique thread wrapping it all ?

@bollu
Copy link
Contributor Author

bollu commented Feb 25, 2015

@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.

@tafia
Copy link
Contributor

tafia commented Feb 25, 2015

Yes
For library purpose, is the canonical way really to have a thread for each
and every function ?
I find it strange. Better have a wrapper for libraries while main directly
could have its own private thread.
No more nested calls.
It still feels a wrong
On 25 Feb 2015 20:14, "Siddharth" [email protected] wrote:

@tafia https://github.com/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.


Reply to this email directly or view it on GitHub
#164 (comment).

@ghost
Copy link

ghost commented Feb 25, 2015

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.

@bollu
Copy link
Contributor Author

bollu commented Feb 25, 2015

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.

@phildawes
Copy link
Collaborator

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:

real    0m0.352s
user    0m0.203s
sys 0m0.147s

after:

real    0m0.628s
user    0m0.300s
sys 0m0.405s

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

@phildawes
Copy link
Collaborator

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.

@phildawes
Copy link
Collaborator

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

@bollu
Copy link
Contributor Author

bollu commented Feb 27, 2015

@phildawes: don't we need the abort_if_errors? I was under the impression that the particular function must be called.

@bollu
Copy link
Contributor Author

bollu commented Feb 27, 2015

I was testing this file against RACER to see what happens when there are errors.

I removed the abort_if_errors() from the with_error_checking_parse(...).

Here's the output:

1 22 tests/errored_rust.rs
bogofile:1:7: 1:9 error: expected one of `;` or `as`, found `td`
bogofile:1 use s td::
                 ^~
thread '<main>' panicked at 'Box<Any>', /home/bollu/prog/rust/src/libsyntax/diagnostic.rs:92
PREFIX 11,11,

The stack trace is here:

thread '<main>' panicked at 'Box<Any>', /home/bollu/prog/rust/src/libsyntax/diagnostic.rs:92
stack backtrace:
   1:     0x7f6a369c4fe0 - sys::backtrace::write::hd237769f574cebd0KlC
   2:     0x7f6a369ed890 - panicking::on_panic::hd026f39bc548b60ftXL
   3:     0x7f6a3692e4f0 - rt::unwind::begin_unwind_inner::h5412fee8459176ffRBL
   4:     0x7f6a3734a920 - rt::unwind::begin_unwind::h11880206013635135768
   5:     0x7f6a372f09a0 - diagnostic::SpanHandler::span_fatal::h69a6eb53eef78d20sSE
   6:     0x7f6a3735c580 - parse::parser::Parser<'a>::expect_one_of::h8e9bf8acfc2488c8xRI
   7:     0x7f6a3735dc10 - parse::parser::Parser<'a>::expect::h93479551fce8f3293PI
   8:     0x7f6a373868f0 - parse::parser::Parser<'a>::parse_item_::hb75827bd2292b996zUN
   9:     0x7f6a37397e60 - parse::parser::Parser<'a>::parse_mod_items::h5395ab0c6c705996PjN
  10:     0x7f6a373a33b0 - parse::parser::Parser<'a>::parse_crate_mod::hf2728e1de9b80fd3FlO
  11:     0x7f6a37e9d280 - racer::ast::string_to_crate::closure.13957
  12:     0x7f6a37e9d0a0 - racer::ast::with_error_checking_parse::h9486131292660222995
  13:     0x7f6a37e9cff0 - racer::ast::string_to_crate::hbb393764668d2f3ahFa
  14:     0x7f6a37ed1af0 - racer::ast::parse_use::h5a175402a9a9e009YZb
  15:     0x7f6a37f4be80 - racer::matchers::match_use::h96b1d2939d885291Bzg
  16:     0x7f6a37f3b3e0 - racer::matchers::match_types::h9ff813c305aa9916bQf
  17:     0x7f6a37f39680 - racer::nameres::run_matchers_on_blob::heabf7272047f6282Abe
  18:     0x7f6a37f35a90 - racer::nameres::search_scope::h83058985f8204811E0d
  19:     0x7f6a37f3d010 - racer::nameres::search_local_scopes::h893dc46214980654vfe
  20:     0x7f6a37f336c0 - racer::nameres::resolve_name::hc4ad6c0d77ef56beTCe
  21:     0x7f6a37f207b0 - racer::nameres::resolve_path::h3320f02c86c3d3f5OJe
  22:     0x7f6a37f5cf20 - racer::complete_from_file::h421ec1888af1fb81pGh
  23:     0x7f6a37f618d0 - complete::hdf9353fba9529135LWh
  24:     0x7f6a37f65660 - main::h9f9740ef5524604819h
  25:     0x7f6a36a64dd0 - rust_try_inner
  26:     0x7f6a36a64dc0 - rust_try
  27:     0x7f6a369ef550 - rt::lang_start::hf1a44dd833e003b5tRL
  28:     0x7f6a37f65c20 - main
  29:     0x7f6a36098dd0 - __libc_start_main
  30:     0x7f6a37e6f670 - <unknown>
  31:                0x0 - <unknown>

looking at the source code, it crashes here, with a call to fatal(). This then goes on to finally call panic! within SpanHandler

So the abort_if_errors() codepath has no impact on the crash here. So removing it has no impact.
we do need threads to sandbox this. Or, someone needs to rewrite libsyntax. But I'm not sure when that's happening :)

@phildawes
Copy link
Collaborator

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

@ghost
Copy link

ghost commented Feb 27, 2015

We could also keep one thread for executing libsyntax and create new one each time the previous panics.

@bollu
Copy link
Contributor Author

bollu commented Feb 27, 2015

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?

@bollu
Copy link
Contributor Author

bollu commented Feb 27, 2015

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.

@phildawes
Copy link
Collaborator

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
Copy link
Collaborator

@bollu
Copy link
Contributor Author

bollu commented Mar 2, 2015

@phildawes: I updated the link - should work now - Here's the URL

@phildawes
Copy link
Collaborator

Hi @bollu, I had a look through the GSOC proposal. It would be cool to have you working on racer!

A couple of things:

  • Calling racer 'completely unoptimized' isn't very accurate (and a little disrespectful), I and others have put many hours into performance. It does some pretty sophisticated searching and type inference across crates and returns results in a fraction of a second.
  • It seems that this proposal places heavy emphasis on caching and serializing to a binary format for performance, but you are not specific about what you plan to cache and serialize, or what the performance goals are.

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.

@phildawes
Copy link
Collaborator

FYI: rust-lang/rust#23857

@phildawes
Copy link
Collaborator

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!

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.

racer crashes when an invalid file is given to it
3 participants