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

[adnan338]

On Sunday, 21 June 2020 at 12:43:32 UTC, adnan338 wrote:
> 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.

It is also worth noting that simply saving the values from a new 
sync block makes the `completed` remain stuck in 0;

auto downloadTask = task!(Downloader.start)(downloader);
		downloadTask.executeInNewThread();
		size_t currentlyCompleted;
		double currentFraction;
		synchronized
		{
			currentlyCompleted = cast(size_t) downloader.completed;
			currentFraction = cast(double) downloader.getFraction();
		}

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