Overprotecting multithreaded code

Running long tasks in a background thread to keep the UI responsive is one of the main purposes of multithreading. A common code pattern for doing so would look like:

procedure TMainForm.BtnClick(Sender: TObject);
begin
  TThread.CreateAnonymousThread(
    procedure
    begin
      DoSomeWork;
      end;
    end).Start;
end;

However, such code often needs to show the results of that long work to the user. Working with the GUI from a background thread is not thread-safe, so such code should be executed in the context of the main thread:

procedure TMainForm.BtnClick(Sender: TObject);
begin
  TThread.CreateAnonymousThread(
    procedure
    begin
      DoSomeWork;
      TThread.Synchronize(nil,
        procedure
        begin
          ShowResults;
        end);
      end;
    end).Start;
end;

Because ShowResults will run in the context of the main thread, the code in ShowResults should be as minimal as possible—read: as fast as possible. Once you enter into the Synchronize (or Queue) block, you are no longer executing code in the background thread, but on the main thread. That means if the code in that block takes a long time to run, it will block the UI thread just the same way it would block it if you were running that piece of code directly in the main thread.

Sometimes, in attempts to avoid threading issues, developers overprotect code in background threads and run more code than is necessary inside synchronization blocks. Thread safety can be hard, especially if the developer has only just started sailing in multithreading waters. While overprotecting code can cause other issues, like deadlocks, generally it is better to protect a bit more code than not to protect enough.

However, I have seen cases where overprotecting literally looks like this:

procedure TMainForm.BtnClick(Sender: TObject);
begin
  TThread.CreateAnonymousThread(
    procedure
    begin
      TThread.Synchronize(nil,
        procedure
        begin
          DoSomeWork;
          ShowResults;
        end);
      end;
    end).Start;
end;

In the above example, no line of code is being executed in the background thread, and everything is running in the synchronization block. Such a coding pattern completely defeats the purpose of using threads, and running it will block the UI just the same as the following code:

procedure TMainForm.BtnClick(Sender: TObject);
begin
  DoSomeWork;
  ShowResults;
end;

Using Queue instead of Synchronize will produce similar effects, as both execute the code in the context of the main thread. The difference is that Synchronize will block the thread while it runs, and the thread will finish running after the Synchronize block is executed, while Queue is a non-blocking call.

Why would anyone write code like that?

It is hard to guess how some pieces of code come to life. In this case, I don't think that happened because the developer was overprotective. The most logical explanation is that the developer in question was completely unaware of what the Synchronize method actually does.

Or maybe there was a misunderstanding about how Synchronize gives you thread safety. If you are unaware that Synchronize executes code in the context of the main thread, calling all code running in a background thread inside a Synchronize block may seem like the logical thing to do. After all, you don't want to have threading problems.

If you take a look at the example again, and imagine that you don't know anything about how Synchronize works, it will seem like all the code that is supposed to run in the background thread will run in the background thread, including the code written in the Synchronize block.

Though it started as a totally unimaginable piece of code, suddenly, you can see why would someone write it like that.