GtkD code review - How to update a progressbar using data sharing concurrency

[adnan338]

On Sunday, 21 June 2020 at 09:16:06 UTC, Ali Çehreli wrote:
> On 6/20/20 9:30 AM, adnan338 wrote:
>
> > Hello, I need a code review on my strategy
>
> I don't know gtkd so I did not compile the code and I did not 
> review the code very carefully.
>
> However, I don't think you need to 'synchronized' the whole 
> parallel loop. Since there is only one thread that executes 
> start(), that synchronized cannot have any effect at all. What 
> you want to synchronize is the mutating access to 'completed' 
> by the threads that parallel() starts automatically. So, move 
> 'synchronized' just around that expression:
>
> // REMOVE this one:
> // synchronized
> // {
>
>   foreach (_; downloader.links.parallel())
>   {
>       Thread.sleep(uniform(0, 6, rnd).seconds());
>
>       // ADD this one:
>       synchronized {
>         ++cast() downloader.completed;
>       }
>   }
>
> // }
>
> Ali

Thank you. However I am concerned about a data race. I have at 
least two places where I am at risk of a data race.

1. In the static method `start`, where I mutably access the value 
`completed` from parallel threads.
     I *think* I have implemented it safely simply by making the 
`completed` a shared(size_t).

2. In the `timeout` delegate. The glib Timout is a struct that 
accepts a delegate and invokes it periodically. The ctor requires 
3 values,
   i. polling time (in ms)
   ii. the delegate to execute in each polling
   iii. priority
   The return value is whether the timeout should continue. The 
gtk event loop executes the triggers timeout automatically afaik.

     I think I am at risk here. Before constructing the Timeout, I 
create a new task and invoke it in a new thread:

auto downloadTask = task!(Downloader.start)(downloader);
downloadTask.executeInNewThread();

Right after that I create a Timeout. I try to read the 
`completed` (which is the shared member as mentioned in the 
previous point) once in every 100 ms.

auto timeout = new Timeout(100, delegate bool() {
			if (downloader.completed < downloader.links.length)
			{
				if (downloader.completed == 0)
				{
					pib.setText(`Awaiting response...`);
					pib.pulse();
				}
				else
				{
					pib.setText(`Downloading...`);
					pib.setFraction(downloader.getFraction());
				}
				return true;
			}
			else
			{
				super.setTitle(`Downloading complete`);
				// pib.setShowText(false);
				pib.setVisible(false);
				return false;
			}
		}, GPriority.HIGH);

I am thinking I am prone to a data race here. The `completed` is 
being updated by the `start` method and also is being read by the 
main thread inside `timeout`

I am trying to figure out how to prevent this data race.