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

Purge CurlSharp #3118

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Purge CurlSharp #3118

merged 1 commit into from
Jul 17, 2020

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Jul 16, 2020

Problem

Some Windows users are seeing this on launch:

screenshot

Unhandled Exception: System.BadImageFormatException: An attempt was made to load a program with an incorrect format. (Exception from HRESULT: 0x8007000B)
   at CurlSharp.NativeMethods.curl_global_init(Int32 flags)
   at CurlSharp.Curl.GlobalInit(CurlInitFlag flags)
   at CKAN.Curl.Init()
   at CKAN.CmdLine.MainClass.Main(String[] args)

Causes

In #3107, we started trying to initialize CurlSharp at startup to avoid thread-unsafe initialization, because the warning message about it was chronically spamming the Discord notifications channel for the NetKAN bot. Apparently CurlSharp needs to load the Visual C++ runtime to do its work, but some users have multiple copies of it in their PATH, which confuses Windows if it's a 32-bit copy:

(The misleading error message blaming the application for this is just one of dozens of ways Windows could do better here.)

Any call to Curl.Init() was supposed to catch and ignore all exceptions, but this wasn't done with the new calls in #3107 (because I didn't know about this requirement/assumption, because that's life with code that relies on exceptions for important behavior). Instead, the missing DLL exception was caught and ignored (which probably causes other problems by preventing the fallback from being skipped during downloads on Windows), and this BadImageFormatException is uncaught.

Background

According to @techman83, the CurlSharp fallback was added the first time a new TLS version requirement broke everything. (The second time was #2293, with which CurlSharp was memorably unhelpful, probably because it's not for Windows.)

The fallback is no longer needed (because .NET and Mono both support the latest TLS) and is an obstacle to refactoring the network code. Any change has to consider the possibility of multiple connections being attempted for the same URL with exceptions thrown, caught, re-thrown, etc. It can be difficult even to identify where a given change should go, and to avoid breaking something.

Workaround

@Dominiquini figured out that clearing the PATH environment variable helps!

C:\Windows\System32\cmd.exe /c "SET PATH= && START /D ^"C:\Program Files (x86)\CKAN" ckan.exe"

(Details may vary, not guaranteed effective on any particular Windows system, use at your own risk.)

Changes

Now all the libcurl stuff is removed. We try to download a URL once with WebClient, and if that doesn't work, too bad.

As well, all projects now have this set for all builds:

    <Prefer32Bit>false</Prefer32Bit>

Previously it was only set for Debug builds, which may have played a role in how these DLLs were loaded.

Fixes #3115.
Fixes #3095.

@HebaruSan HebaruSan added Core (ckan.dll) Issues affecting the core part of CKAN Pull request Windows Issues specific for Windows Network Issues affecting internet connections of CKAN labels Jul 16, 2020
@HebaruSan HebaruSan requested a review from DasSkelett July 16, 2020 21:23
Copy link
Member

@DasSkelett DasSkelett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tested a bunch of different download scenarios, registry update, client update check, and mod downloads, also while forcing connection errors: I think we didn't break anything.

I can also confirm that ckan.exe in release configuration runs in 64bit mode now.

Copy link
Member

@techman83 techman83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A cursory check in prod and it looks good to me. Curl was added to work around Mono's lagging in the TLS department, this is no longer the case and the project is well out of the woods in regards to how far behind the distros were with their packaging.

It adds complexity and hasn't been useful for quite some time, good work unpicking all that!

.github/workflows/build.yml Outdated Show resolved Hide resolved
Dockerfile.netkan Outdated Show resolved Hide resolved
@HebaruSan HebaruSan merged commit d07a640 into KSP-CKAN:master Jul 17, 2020
@HebaruSan HebaruSan deleted the fix/rm-curl branch July 17, 2020 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core (ckan.dll) Issues affecting the core part of CKAN Network Issues affecting internet connections of CKAN Windows Issues specific for Windows
Projects
None yet
3 participants