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

File Structure recovery sometimes fails #1

Closed
oleg-shilo opened this issue Apr 7, 2017 · 9 comments
Closed

File Structure recovery sometimes fails #1

oleg-shilo opened this issue Apr 7, 2017 · 9 comments

Comments

@oleg-shilo
Copy link
Owner

Based on bruderstein/nppPluginManager#52

It looks like the 1.7.1 work around is not reliable.

@oleg-shilo
Copy link
Owner Author

Ported from: CollinChaffin oleg-shilo/cs-script#62


After installing the prerelease of 1.7.3.07, I really do need help to debug this because the Roslyn support is still not working properly.

Here's what I've done to simplify this because I am getting a bit frusterated:

Removed the env variable for CSScript as I don't want the non-prerelease via chocolately to interfere with this test. I assume the NPP plugin should use the embedded assembles and intellisense executables etc.
Moved EVERYTHING for plugins out of APPDATA temporarily and back to the default programfiles86 and renamed the xml flag file so that NPP is running in default plugin config mode
Installed 1.7.3.07 via unzip manually into plugins folder

Upon executing NPP now, and every execution, I receive this:
snag_4-9-2017_16-03-10

However, there is a change because now, my plugin config the Rosylyn checkbox for some reason remains enabled continually, despite it not working, whereas previous versions upon relaunch the checkbox would be unchecked:
snag_4-9-2017_16-03-20

As you can see, it maintains the setting to utilize the embedded engine to keep this simple:
snag_4-9-2017_16-04-45

...and I am not certain what other steps I can take to fully debug this plugin, but my logic pushed me to execute the "CompatibilityTest.exe" that, unlike before was throwing an exception and crashing hard, now says all is great (just to take the other half of my hair):
snag_4-9-2017_16-09-44

So, @oleg-shilo, I really appreciate your assistance to help me enable the Roslyn C#7 support.

@CollinChaffin
Copy link

So is there a debug setting I can enable to tell me exactly what exception is being thrown and preventing it from loading? Everything seems okay otherwise so I am at a loss as to why this is happening now after eliminating the first 2 reasons (folder structure, and APPDATA location) that I have totally eliminated.

@oleg-shilo
Copy link
Owner Author

It is a complex problem and below is the full overview of the symptoms, causes and the history.


Plugin before v1.7.0

Prior introduction of C#7 support (v1.7.0) the plugin was distributed with two different versions of Roslyn binaries. This is due to the fact that before VS2017 release Roslyn (v1.0.3) was in Beta for scripting functionality. And to make the matter worth, the versions that worked for scripting were in conflict with versions that worked for code analysis. CS-Scriupt.Npp solved this problem by distributing both conflicting distros in their individual sub-folders (Roslyn and Roslyn.Intellisense) with the plugin implementing distro specific assembly probing.

Later, Notepad++ Plugin Manager (PM), at least v1.4.3.0, has introduced the defect - collapsing all plugin sub-folders into one single (root) directory, effectively completely screwing Roslyn. Currently this issue is still being worked on by Npp team (bruderstein/nppPluginManager#52).

Plugin v1.7.0

CS-Scriupt.Npp v1.7.0 - was released with a single Roslyn folder (<root>/Roslyn) as the latest Roslyn removed the internal incompatibility. Though, because the plugin was still relying on the presence of the Roslyn sub-folder the faulty PM was still preventing C# 7 from working,

Plugin v1.7.1

CS-Scriupt.Npp v1.7.1 - was a desperate attempt to deal with the faulty PM. It has introduced additional assembly probing flow for the "collapsed file structure". It also introduced an aggressive "Fix file structure" feature, which later was found to be unreliable.

Plugin v1.7.3

CS-Scriupt.Npp v1.7.3 - was released after Npp support indicated that the PM fix is to be delivered (sooner or later). This version has revoked the over ambitious Fix file structure feature, improved error handling/reporting and limited auto-disabling C#7 support (in case of Roslyn error) to the session scope only.

Plugin v1.7.4

During the intensive testing of plugin v1.7.3 the latest Roslyn (v2.0.0) has unexpected assembly probing behavior presumably triggered by being hosted by in unmanaged hosts (e.g. Notepad++). Thus user assembly probing sometimes was not invoked on the obvious default probing failures. These findings are consistent with findings during the integration of CS-Script intellisense engine with Sublime Text on Linux where user assembly probing was not triggered for some assemblies all the time. This is most likely due to some custom assembly probing implemented by Roslyn itself.

While the the problem was only intermittent (~1 out of 20 in the testing environment) it had to be addressed. Release v1.7.4 has moved RoslynIntellisense.exe (the only consumer of Roslyn binaries) into the Roslyn sub-folder. This is expected to prevent custom assembly probing at all as the default probing would be up to the challenge. This changed made the problem go away in the test environment but still needs to be confirmed by users.

Thus the v1.7.4 is the latest version that needs to be used for C# 7. Unfortunately the plugin's update channel (in AboutBox) may or may not deliver it correctly so the best way is to install the plugin manually.

@oleg-shilo
Copy link
Owner Author

Hi @CollinChaffin, the post above explains the issues. My guess is that you were still testing v1.7.3-pre. If so can you please do the testing again but now with v1.7.4-pre. I expect it to fix the issue completely.

@oleg-shilo
Copy link
Owner Author

oleg-shilo commented Apr 10, 2017

As for error logging it's always enabled. You can access it from AboutBox:
image

Though it doesn't help much as the exception is indicating that the Roslyn failed to do proper assembly probing by some how bypassing CS-Script pugin's asm probing routine. BTW this is the reason why CompatibilityTest.exe is successful - CompatibilityTest.exe is a managed not native host.

Anyway, v1.7.4 should address this final bit.

@CollinChaffin
Copy link

Okay boy I wish I had seen that 20hrs ago I probably spent about 10 more screwing around with this :) LOL. Anyway, good news is, it's fixed. Holy cow. It works again! Whew!

I am curious I guess I can read the commit diffs your explanation above doesn't quite align with my testing because for me on mult systems, it was 100% of the time failure, bar none. At NO time can I get 1.73 Roslyn support to work down to fresh VM OS installs, zero other NPP plugins on fresh NPP installs - I mean I beat this to death so I'm guessing there had to be (something) absolutely broken that absolutely looks like at a glance (of the changelog) was fixed by basically moving the Roslyn files again.....which STILL doesn't seem to align since in my last hours of screwing around with this, I was literally moving those assemblies myself just to see if in fact it was still a directory location issue - unless I just missed whatever magic location change occurred. But, since we're only talking about 3 folders, the NPP main plugin, the CS-Script plugin, and then the Roslyn subdir - I'm still a bit confused what this ultimate fix was.

So if you get a min I'd love a bit more info on what you changed from 1.73 to 1.74 pre and what your take is on the fix etc. I'd really appreciate it I only ask since I invested so much to try to help with this and it really does interest me. BTW, thanks for the acknowledgement! I do appreciate that! Also, hopefully as I get more free time in the hopefully near future, I'd be interested in perhaps helping you with some additional pulls/actual code contributions too if you'd be interested when the time comes!

@oleg-shilo
Copy link
Owner Author

oleg-shilo commented Apr 10, 2017

Okay boy I wish I had seen that 20hrs ago I probably spent about 10 more screwing around with this :)

Mate, I know the pain. :) I also spend my whole weekend on this. On something that shouldn't' have happened in the first place. Something that ultimately you have no control over.

Anyway I am so relieved that it worked in your environment as your 100% is a way more trustworthy comparing to my 5% failure rate.

I'm still a bit confused what this ultimate fix was.

This is what is the fix (actually work around) in v1.7.4.

The latest Roslyn (v2.0.0, bundled with cs-script.npp v1.7.0+) must have introduced something that screwed with the user assembly probing. My AppDomain.AssemblyResolve event handler is not called in 5% of the runs (100% in yours). Mind you on Linux I also have 100% failure rate.

The invocation of the user asm resolving algorithm is non-deterministic. CLR may not fire it if other AssemblyResolve returned non-null result. Even if the result is wrong. My speculation is that this is exactly what Roslyn started doing from v2.0.0.0. Though I have no evidence of that.

Anyway, Nothing can be done to fix it. Roslyn is doing his business and there is nothing wrong from its point of view. Thus instead of fixing the compromised asm probing in the RoslynIntellisesnse.exe (CS-Script.Npp module that loads/uses Roslyn) I brought this module inside of the Roslyn directory so it doesn't need to do the probing any more.

This change potentially requirs a new asm probing from RoslynIntellisesnse.exe for Intellisense.Common.dll. But it is simple enough as I have full control over the both modules. And on practical level probing is not even needed as Intellisense.Common.dll is always loaded before RoslynIntellisesnse.exe.
image

image

The bottom line is that we had two fundamental problems:

  • Npp collapsing the folders
    Still not fixed. User has to install/restore the plugin manually.
  • Roslyn obstructing asm probing from RoslynIntellisesnse.exe
    Solved by avoiding asm probing as the result of moving the module inside of the Roslyn folder.

Thank you again for spending time on this. I will start pushing the release notifications to the Npp users.

@CollinChaffin
Copy link

Hey so sorry haven't gotten back to you on this! Just caught up and it makes better sense now - thanks for the additional explanation clearly we both were looking at this one FAR too long! :)

It's just awesome we were able to debug through it - that is always very rewarding when the debugging at least leads to a fix! I'm just catching up on the plugmgr too I see your last questions about the reinstall/ininstall etc. and it's funny in my last test runs I actually wrote down that exact list of questions. I certainly wasn't trying to step on your toes and I didn't mean to come across not understanding how much work you've put into this I absolutely do! As I said over in that issue #52, because my only real outstanding questions from a testing perspective are the exact ones you already asked, I'll pull back a bit as to not get in your way of the development effort - but, by all means anytime you could use a second set of eyes or hands on an this or any other issue don't hessitate to ping me and I'll see if I can help.

I'm happily using the latest pre (or unplublished) version 1.7.6 and in the installed/running state it's working great! Once again, thanks again for the release note shout-out on helping with the issue, I always appreciate seeing other DEVs do that I've always tried to do that myself when other DEVs have helped me - but as I'm sure you're aware - most DEVs unfortunately do not these days so dealing with another member of a dying breed is a nice change!

@oleg-shilo
Copy link
Owner Author

Hi Collin,

Yes it has been an interesting journey. And what is really rewarding is that he solution is something that truly addresses the problem but not just decreasing its chances. And the fact that the problem shouldn't exist at the first place is kind of irrelevant.

I also wanted to thank you for all the time you spent on this and the fact that you have up-voted the PM defect I logged. I do believe it made the difference. Suddenly after 2 weeks if being effectively ignored it got the attention it deserved. That is why I felt the need to mention your assistance in the release notes. :)

I see your last questions about the reinstall/ininstall etc. and it's funny in my last test runs I actually wrote down that exact list of questions...

As for this one, I decided to abandoned any effort in this direction. The NPP guys are so busy that they just have no time to answer the questions properly and promptly. I have already learned that if the question doesn't get simple answer quickly then you may be facing the prospective of getting into the slow unproductive endless question/answer exchange. With no warranty of getting the complete answer ever. I do not blaming anyone I am just trying to be realistic and be practical about the solution to the problem.

Thus I decided to limit my dependency on Npp and implement unconditional "run-on-uninstall" step from inside of my plugin. I was able to do this simply because PM will always stop the NPP before the uninstall and the plugin can hook up to the NppOnExit event.

This technique is not truly for "OnIninstall" but rather "OnPossbleUninstall" but it is good enough for my case: I just need to kill VBCSCompiler.exe that was started by the exiting Npp anyway. The approach is not ideal and it can interfere with other VBCSCompiler.exe clients (CS-Script, Sublime Text, Visual Studio). However the probability of this is low and the impact is almost undetectable - one off ~5 seconds delay with the C#/VB code compilation.

Thank you again for your help.

Happy Easter to you and your family.

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

No branches or pull requests

2 participants