wimh/SparkleShare — comment on commit 9d6cc36ad18b0938245e96c85cd0b976ada7cad5
File:  line 0
By wimh:

This is a fix of a workaround I have added. I explain the problem I had first:

`PopulateRepositories` is [started in a thread][1]:

new Thread (new ThreadStart (PopulateRepositories)).Start ();

[`PopulateRepositories`][2] creates `Repositories` and calls `AddRepository` to add a repository to the `List`:

private void PopulateRepositories ()
{
Repositories = new List ();

foreach (string folder_name in SparkleConfig.DefaultConfig.Folders) {
string folder_path = new SparkleFolder (folder_name).FullPath;

if (Directory.Exists (folder_path))
AddRepository (folder_path);
else
SparkleConfig.DefaultConfig.RemoveFolder (folder_name);
}
...

This can potentially cause two problems.

Because `PopulateRepositories` is called in a thread, the main program continues. Functions like `UnsyncedFolders`, `GetLog` and `UpdateState` use `Repositories`. Those functions include code like:

foreach (SparkleRepoBase repo in Repositories) {

The main program calls some of those functions sometimes before `PopulateRepositories` has been finished. At least I had a crash because of this.

Problem 1: If the thread starts very slow, `Repositories` is still null when for example `UnsyncedFolders` is called. The foreach will throw an exception.

Problem 2: `Repositories` is a `List<>`. A `List` is not [thread safe][3]. So when 'AddRepository' adds something to the list, the foreach can fail if it is reading the same time.

It has been some time ago when I found this problem. So I am not sure why I didn't take some time to create a good fix, but my temporary fix has two parts:

I create `Repositories` [during construction][4];

public abstract class SparkleControllerBase {

public List Repositories = new List ();

And [in `PopulateRepositories`][5] I use a temporary list which `AddRepository` uses, and when the list is complete, I assign it to `Repositories` to avoid threading issues:

private void PopulateRepositories ()
{
List TempRepositories = new List ();

foreach (string folder_name in SparkleConfig.DefaultConfig.Folders) {
string folder_path = new SparkleFolder (folder_name).FullPath;

if (Directory.Exists (folder_path))
AddRepository (TempRepositories, folder_path);
else
SparkleConfig.DefaultConfig.RemoveFolder (folder_name);
}

Repositories = TempRepositories;

But this got broken, probably because of a previous merge. this commit fixes it again.

I did not create a pull request yet, because I wanted to create a better fix first.

[1]: https://github.com/hbons/SparkleShare/blob/master/SparkleShare/SparkleControllerBase.cs#L154
[2]: https://github.com/hbons/SparkleShare/blob/master/SparkleShare/SparkleControllerBase.cs#L640
[3]: http://stackoverflow.com/questions/703990/why-is-c-sharp-list-not-thread-safe
[4]: https://github.com/wimh/SparkleShare/blob/9d6cc36ad18b0938245e96c85cd0b976ada7cad5/SparkleShare/SparkleControllerBase.cs#L39
[5]: https://github.com/wimh/SparkleShare/blob/9d6cc36ad18b0938245e96c85cd0b976ada7cad5/SparkleShare/SparkleControllerBase.cs#L649