Avoid registering the same scheme multiple times #543
Conversation
|
Yes this is an annoying error message, but not one that actually hurts us, as it's on a promise that we don't wait on, but I don't think this is the right fix. As it's in essence a race between parallel registrations of the VFS on the VS Code side. In regular rascal mode:
most cases, 1. wins (as it tends to get started first). you don't see this message In deployed mode:
These are all started in parallel, and the 2 instences don't know of each other, or at least, they don't know if they're the only one thats there. (there is also the edge case of where a scheme might already by another vs code extensions, but let's ignore that). So, to make sure rascal schemes work in VS Code (so you can open a
And the only way we could check if maybe someone else already did register a certain scheme is the I see 2 strategies to get rid of this message:
So to be clear, the current fix is a workaround, that will work in some settings (for example if all the parametric servers share the same instance of the VFS server) but it won't work in a different setting where there exists multiple rascal based deployed extensions, where they do not share the VFS client refence and thus, the list of already registered schemas. |
It hurts a little bit. We are in the process of adding telemetry and these constantly show up as |
Ah, yes that's annoying, so catching it might be the easiest? Or do the check first (but still might loose the race)? Both should be a small fix in the code? Are you okay updating the PR with these fixes? |
Yes, I will. |
|
Thanks @urbanfly. I've been thinking about the code a bit more,it's not just a pure race, right now we just always register them, without checking if we've already done it. My classification of a race was more: if you want to solve this, you get into messy races. |
Can this be an optional feature that the consumer can opt out of w/ a new parameter on Some ext. probably don't need this ability. And if there are multiple |
|
The cases that break would be very subtle, I rather we don't get into that. It really depends on the kind of locs you're using, and sending to VS Code.
Ah right, I thought I did something like that already. Okay, so it's a proper race now, between that check and the subsequent register. So just catch the exception is the wisest thing, we could even skip that whole preflight check and just catch the error? That should solve it once and or all. |
|
I changed the PR to now attempt to register all schemes and log (info) when they succeed and log (error) when they fail. These log messages show up in the language-specific OutputChannel. Example of log output
or This does change the behavior slightly - when the original code failed to register |
urbanfly
force-pushed
the
feature/avoid-zip-errors
branch
from
b8981ee to
df97a61
Compare
There was a problem hiding this comment.
Looks good, let's just try and reduce the logging output a bit? People get confused when there are many log messages that say something broke. (I fully understand it's useful for debugging this fix).
I like the behavior that we continue even if one failed, much nicer 👍🏼
| new RascalFileSystemProvider(client).registerSchemes(schemes); | ||
| const allRegistered = new RascalFileSystemProvider(client).tryRegisterSchemes(schemes); | ||
| if (!allRegistered) { | ||
| client.warn(`At least one of the expected schemes failed to register.\nExpected schemes: ${schemes}`); |
There was a problem hiding this comment.
not sure if this should be a warning, as we expect races? So sometimes it happens, and that's fine?
| this.client.info(`Scheme registered: ${s}`); | ||
| return true; | ||
| } catch (error) { | ||
| this.client.error(`Unable to register scheme: ${s}\n${error}`); |
There was a problem hiding this comment.
Can we tweak this a bit to reflect that most likely you can ignore this error?
| .filter(s => !this.protectedSchemes.includes(s)) | ||
| // we add support for schemes that look inside a jar | ||
| .concat(schemes |
There was a problem hiding this comment.
ah yes, that's nice :) just concat them 👍🏼
When multiple
ParameterizedLanguageServerinstances are created in the same extension, the same schemes are registered multiple times - failing.This PR avoids registering a scheme multiple times by filtering the previously registered schemes.
VS Code API docs say: