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

Fix freeze on non CKAN files in cache folder #2743

Merged
merged 1 commit into from
Apr 28, 2019

Conversation

HebaruSan
Copy link
Member

@HebaruSan HebaruSan commented Apr 27, 2019

Problem

Some users reported that CKAN freezes while trying to load the registry at startup.

Cause

This exception is thrown:

System.ArgumentOutOfRangeException: 인덱스 및 길이는 문자열 내의 위치를 참조해야 합니다.
매개 변수 이름: length (Rough translation: The index and length must refer to the position within the string. Parameter name: length)
   위치: System.String.Substring(Int32 startIndex, Int32 length)
   위치: System.Linq.Lookup`2.Create[TSource](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   위치: System.Linq.GroupedEnumerable`3.GetEnumerator()
   위치: System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   위치: System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector)
   위치: CKAN.NetFileCache.GetCachedFilename(Uri url, Nullable`1 remoteTimestamp)

... which means that this block from #2563 tried to get a substring of a filename shorter than 8 characters:

cachedFiles = files = allFiles()
.GroupBy(fi => fi.Name.Substring(0, 8))
.ToDictionary(
grp => grp.Key,
grp => grp.First().FullName
);

It appears that some users may have chosen a CKAN cache folder that is not exclusive to CKAN:

screeenshot

Changes

Now that block will ignore any filenames that don't start with 8 hex digits and a dash. This will both fix the problem with short filenames and exclude other files that were not created by CKAN even if they have long names.

Fixes #2711 (I think).

@HebaruSan HebaruSan added Bug Core (ckan.dll) Issues affecting the core part of CKAN Pull request labels Apr 27, 2019
@Olympic1 Olympic1 removed the Bug label Apr 27, 2019
@HebaruSan HebaruSan added the Bug label Apr 27, 2019
@DasSkelett
Copy link
Member

Oh wow, didn't think of the possibility that there are other files with shorter names in the cache folder.

There's another issue happening:
Since v1.26, more precisely #2617, (fatal) errors are no longer thrown during startup. This is due to the whole process being a task now, and C# doesn't throw errors happening in tasks without using await.
This can be a big problem, because the only way to get a stack trace and error message is with a debugger, which is only an option if you know how to reproduce a bug. In #2711 this was a problem to, we only got the error text with a downgrade to v1.25.
I'll do a PR to fix this.

@HebaruSan
Copy link
Member Author

Oh, oops. I assume we can keep the loading screen if we switch to async/await?

@DasSkelett
Copy link
Member

DasSkelett commented Apr 28, 2019

Yes. It works exactly the same, but actually throws errors if they occur.
It's nothing more than a 2-line change, but see yourself in a few minutes, when my PR is done.

@HebaruSan
Copy link
Member Author

OK, good. I've tried probably around five times now to learn async/await, and I still find threads much easier to understand.

@HebaruSan HebaruSan merged commit 579da24 into KSP-CKAN:master Apr 28, 2019
@HebaruSan HebaruSan deleted the fix/cluttered-cache-freeze branch May 9, 2019 01:29
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CKAN stuck at loading available modules
3 participants