Typing special characters into the workspace symbol search causes language service to crash

[devoncarew]

To repro:

• open the type dialog (cmd-T)
• search, close, search, close, search, ...
• after a few iterations, I no longer get results (the indeterminate progress indicator doesn't terminate)

Perhaps as a result of the work to combine APIs? Feel free to close if this isn't reproducible.

[DanTup]

Unable to repro this after 10-15 searches. I wonder if it might be related to what you say with #62. Can you repro either?

I'm happy to revert the merging to do a release of the other stuff if it seems like it might be related (it is a bit weird, it waits for two promises, but I didn't think there was much to go wrong there).

I'll try doing some testing; maybe it one of the promises rejects something bad happens?

[devoncarew]

Perhaps it's specific to my project? I'll close until I can get a more specific repro.

[DanTup]

If you think there's definitely an issue here, I'm happy to keep this open. If there are bugs, I'd like to find them :-)

So even if you struggle to get a reliable repro, if you see it again, do re-open. We can add more diagnostics if required to help track it down.

[DanTup]

Re-opening this and putting in 0.8; would like to investigate this further. Whatever the cause (unless you terminated dart.exe ;)) we probably can/should be able to handle it better. Currently there's not a lot of handling of error responses, etc.

[devoncarew]

Just FYI, I was not able to repro when I re-tested this.

[DanTup]

Ok, closing for now. If you do see it again, do post back (even if you can't repro) so we can maybe add some logging or something.

[DanTup]

Just had this too. I've turned on logging and it seems nothing is going to the AS when I'm typing in the search window. Seeing what I can find...

[DanTup]

Think I repro'd... looks like a search never returned:

Starting search for \|,./<>?;'#:@~[]{}-=_+
Sending top level search for \|,./<>?;'#:@~[]{}-=_+!
Sending member search for \|,./<>?;'#:@~[]{}-=_+!
Got results for member search for \|,./<>?;'#:@~[]{}-=_+!

[DanTup]

@devoncarew Yep, can repro by pasting \|,./<>?;'#:@~[]{}-=_+! into the search! Possibly I (we) typed a strange character that caused it?

Maybe related to regex in searchTopLevelSymbols (we only filter out a small number of characters?). I'm off for the night, but should be able to quash this tomorrow :)

[DanTup]

Probably the request comes back with an error, but we don't handle it. So probably there are a few things to do:

1. Add a function to drop failed responses to console (like this)
2. Fix issue with escaping
3. Ensure when the merged promise always gets completed when the second request completes, whether either (or both) were successful or not.

It's possible this (bad chars) isn't the exact cause, but clearly the handling of errors with the split results is bad!

[devoncarew]

Yeah, running that search locally, the analysis server responds with an error. We should be sure to error out the original vscode request (or complete w/ no results).

[DanTup]

If one request errors and one succeeds, you think we should drop the whole thing and not return the results we did get? (I have no real preference, as I'd like to eliminate them entirely(!), but we need to do something to stop this in the case there are problems).

[devoncarew]

I think it'd be reasonable to fail on an error from either -

[devoncarew]

OK, looking into this, the server is not returning a result from search.findMemberDeclarations - either a success or an error. It throws an exception internally when trying to parse the regex. The search.findMemberDeclarations method needs to guard against a bad regex (search.findTopLevelDeclarations already does this).

The fix for this would be similar to dart-lang/sdk@c52843e#diff-e160143856c218a25071e87f473e833e.

[DanTup]

Ah, yes. Doh! Brian mentioned actually mentioned this:

Note that search.findMemberDeclarations takes a name (such as 'length'), which makes it unsuitable for this purpose as designed. However, the implementation currently builds a regular expression by adding anchors at the beginning and end of the passed string to ensure that we don't match partial names, but neglects to escape any characters in the name itself. I don't think this was our intent, and we reserve the right to fix this in the future, but I think if you pass in '.*' as a name that we'll match every member name. (It's only because of this bug that you have any hope of using it to get the information you want.)

Though strangely in my log this one did come back, it was top-level that didn't! I'll try and dig into it later.

[DanTup]

Ok, I've filtered out some more chars and applied the filtering to both searches; seems to have resolved the issue. Btw - I presume the crash I see when sending a bad query to findMemberDeclarations is what you fixed in the linked changeset, and we don't need to handle that case (where a search fails by result of a server error notification rather than an immediate rejection)? (the id seems to only be included in a string, so it would be hard to reject based on that).

Additionally, I pushed this:

5b51d0e

Previously I didn't propagate failed requests properly, so if Code ever relied on the promise being completed weird things could happen (I think Code is automatically rejecting them after a few seconds, but if we can fail earlier it gives faster feedback).

I know realise this issue wasn't as a ersult of us never completing the promise, but actually that the AS was crashing (the error had isFatal: true). All makes a bit more sense now!

Closing this as I believe it's done. If you see any similar weird behaviour you think may be related to this fix (or the propagation of rejections), shout!