Multithreading is (still) hard!

Multithreading is hard!

No matter how long you deal with it and how good you (think) you are, you will make a mistake. Usually, it will be a problem that will only exhibit itself in a rare circumstances, most probably on a hard-to-reach customer machine. With some (bad) luck it will only appear on Friday afternoons or during your vacation.

That is why I'm always introducing multithreading with the "Don't do it yourself!" motto. Use a standard library! (And by that I mean OmniThreadLibrary, of course. ;) ) As your code, it too will have bugs. Unlike your code, it has 1000+ users running it in very different environments which means that at least it is tested as much as possible.

There are, nevertheless, bugs that escape detection for a long time. In 2011, for example, I fixed a well-hidden problem in TOmniBlockingCollection ("Multithreading is Hard!"). As nasty as that was, it was nothing compared to the bug I found recently!

As it turned out, the implementation of a bounded (fixed-size) multiple-producer multiple-consumer lock-free queue TOmniBoundedQueue (try saying that in one breath!) was buggy since its inception! As this queue is basis to all OmniThreadLibrary communication channels it is really surprising that the bug hid from everybody for 9 (yes, nine!) long years.

IOmniPipeline termination bug

There was a nasty race condition in the IOmniPipeline termination code which could result in access violations. It has been fixed in the SVN so please update to the latest version if you are using Parallel.Pipeline, Parallel.BackgroundWorker, or Parallel.ForkJoin.

Great thanks go to Dean from DeNovo Software who narrowed down the problem and make sure I can repeat it on my machine.

OmniThreadLibrary 3.03a

OmniThreadLibrary 3.03a has just been released. This is purely a bugfix release which fixes ugly problem in the ForEach abstraction. If the code has called ForEach with empty input set (for example, ForEach(0, –1)), the program would block infinitely.

OTL 3.03a is available via SVN or as a ZIP archive.

OmniThreadLibrary 3.03–quick fix

Yesterday’s 3.03 version was somehow corrupted (in a way that it doesn’t compile). I have no idea what I did wrong as I did recompile test apps before committing and they did compile. Anyhow, if you’re one of the first 85 downloaders, please download the corrected version or implement a very simple fix. In the OtlParallel unit on the line 1663 a class prefix is missing. Just type it in and everything will work.

class procedure Parallel.ApplyConfig(const taskConfig: IOmniTaskConfig; ...

The management apologizes for the inconvenience. ;)

EDIT 2013-10-16: It has been brought to my attention that both the original 3.03 and yesterday's fix were missing the XE5 packages. This has been now fixed. You can download missing packages from here.

TStreamAdapter.Seek – Apology

Sometimes you write a blog post that you wish you wouldn’t. Maybe you don’t, but I certainly did. Yesterday I wrote about TStreamAdapter.Seek Broken For Large Files I completely incorrectly stated that “the bug was not fixed in XE4/XE5”.

The reason for my mistake was that although I verified the problem in the XE2, I didn’t write any test code for it (we already had a failing application so I didn’t have to – or so I thought) and I only checked the RTL for changes. I found out that the TStreamAdapter.Seek didn’t change from XE2 to XE4/5 and assumed the worst.

Well, the good guys at Embarcadero did fix the problem in the meantime – but in a different place. They have added a new Seek overload to the TStream class and this new Seek (which is implemented as an inline function) acts as an intermediary between the TStreamAdapter and the correct TStream.Seek method. This solution is much better than our quick fix as it also fixes all other code in the world which mistakenly calls the wrong Seek overload.

function Seek(Offset: Longint; Origin: Word): Longint; overload; virtual;
function Seek(const Offset: Int64; Origin: TSeekOrigin): Int64; overload; virtual;
function Seek(const Offset: Int64; Origin: Word): Int64; overload; deprecated; inline;

function TStream.Seek(const Offset: Int64; Origin: Word): Int64;
begin
  Result := Seek(Offset, TSeekOrigin(Origin));
end;

Thanks to the Arioch’s prompt in the original post I did write a test code today and when I finally tested it on the XE5, it run just fine! Imagine my shame. :(

What have I learned today? That looking at the source code is not enough, especially if you want to complain. Always write a test case (and then run it!).

I deeply apology to Embarcadero programmers (especially to the guy who fixed the original error) and to you, my readers.

And now, if you’ll excuse me, I’ll pull a cover over my head and gently weep.

TStreamAdapter.Seek Test

This is simple test code which tests TStreamAdapter.Seek functionality and doesn’t require creating 4+GB files on the disk. A fix is also included.

program SeekTest;

{$APPTYPE CONSOLE}

{$R *.res}

uses
  Windows, SysUtils, Classes, ActiveX;

type
  TTestStream = class(TStream)
    function Seek(const Offset: Int64; Origin: TSeekOrigin): Int64; 
      overload; override;
  end;

var
  GSeekOffset: int64;

(* quick fix
type
  TStreamAdapter = class(Classes.TStreamAdapter)
  public
    function Seek(dlibMove: Int64; dwOrigin: Integer; 
      out libNewPosition: Int64): HRESULT; override; stdcall;
  end;

function TStreamAdapter.Seek(dlibMove: Int64; dwOrigin: Integer; 
  out libNewPosition: Int64): HRESULT;
var
  NewPos: LargeInt;
begin
  try
    if (dwOrigin < STREAM_SEEK_SET) or (dwOrigin > STREAM_SEEK_END) then
    begin
      Result := STG_E_INVALIDFUNCTION;
      Exit;
    end;
    NewPos := Stream.Seek(dlibMove, TSeekOrigin(dwOrigin));
    if @libNewPosition <> nil then
      libNewPosition := NewPos;
    Result := S_OK;
  except
    Result := STG_E_INVALIDPOINTER;
  end;
end;
*)

function TTestStream.Seek(const Offset: Int64; Origin: TSeekOrigin): Int64;
begin
  GSeekOffset := Offset;
  Result := Offset;
end;

var
  ts: TTestStream;
  sa: TStreamAdapter;
  np: int64;

begin
  ts := TTestStream.Create;
  sa := TStreamAdapter.Create(ts, soOwned);
  sa.Seek($123456789, soFromBeginning, np);
  sa.Free;
  if GSeekOffset = $123456789 then
    Writeln('Seek is OK')
  else
    Writeln('Seek is broken');
  Readln;
end.

TStreamAdapter.Seek Broken For Large Files

EDIT I was completely wrong in my analysis and this bug was indeed fixed in XE4. I'm leaving the original (slightly edited) post below - firstly to remind myself to thoroughly test everything before posting and secondly to help some poor programmer who will run into the same problem with Delphi XE, XE2, XE3 ... in the future.

---

Yesterday a colleague informed me that he added a workaround for a VCL bug (which we confirmed to exist in XE2, XE4 and XE5) to our codebase and asked me if I could “inform the authorities”, i.e. enter it into QualityCentral. Imagine my surprise when I not only found the bug already entered but marked as fixed!

The problematic code is posted below, problem marked with a yellow color as it is hard to spot.

function TStreamAdapter.Seek(dlibMove: Largeint; dwOrigin: Longint;
  out libNewPosition: Largeint): HResult;
var
  NewPos: LargeInt;
begin
  try
    if (dwOrigin < STREAM_SEEK_SET) or (dwOrigin > STREAM_SEEK_END) then
    begin
      Result := STG_E_INVALIDFUNCTION;
      Exit;
    end;
    NewPos := FStream.Seek(dlibMove, dwOrigin);
    if @libNewPosition <> nil then libNewPosition := NewPos;
    Result := S_OK;
  except
    Result := STG_E_INVALIDPOINTER;
  end;
end;

TStream implements two Seek methods (below). The former is here for backwards compatibility and allows only seeking in 32-bit-addressable (i.e. smaller than 4 GB) files. The latter supports files larger than 4 GB.

function Seek(Offset: Longint; Origin: Word): Longint; overload; virtual;
function Seek(const Offset: Int64; Origin: TSeekOrigin): Int64; overload; virtual;

If you Ctrl+Click on ‘Seek’ in the TStreamAdapter.Seek implementation, IDE will jump to the latter Seek – which is incorrect as in reality the former (32-bit) seek is called. That makes the problem harder to spot.

Embarcadero people – can somebody please reopen the original bug report? This is something that surely needs to be fixed.

Our temporary (we’ll see for how many future Delphi versions) fix is the same as in the QC #105985 – make a copy of the TStreamAdapter.Seek and cast the dwOrigin parameter into TSeekOrigin enumeration so that the correct TStream.Seek method is called.

type
  TStreamAdapter = class(Classes.TStreamAdapter)
  public
    function Seek(dlibMove: Int64; dwOrigin: Integer; out libNewPosition: Int64): HRESULT; override; stdcall;
  end;

function TStreamAdapter.Seek(dlibMove: Int64; dwOrigin: Integer; out libNewPosition: Int64): HRESULT;
var
  NewPos: LargeInt;
begin
  try
    if (dwOrigin < STREAM_SEEK_SET) or (dwOrigin > STREAM_SEEK_END) then
    begin
      Result := STG_E_INVALIDFUNCTION;
      Exit;
    end;
    NewPos := Stream.Seek(dlibMove, TSeekOrigin(dwOrigin));
    if @libNewPosition <> nil then
      libNewPosition := NewPos;
    Result := S_OK;
  except
    Result := STG_E_INVALIDPOINTER;
  end;
end;

Nasty COM regression in XE2

David Heffernan found a nasty RTL bug in XE2 COM implementation.

Is COM broken in XE2, and how might I work around it?

I’ve found a workaround (see the link above), but it is ugly and this should really be fixed in the next XE2 update so please – vote on the QC #100414!

Update: XE2 Update 2 fixes the bug described above. Great response time from Embarcadero!

 

Multithreading is Hard!

I have known for a long time that there’s a potential race condition between two writers in TOmniBlockingCollection but I thought that it doesn’t present and clear and present danger. Boy was I wrong!

How to crash Delphi compiler in four easy steps

Step one

Start Delphi XE.

Step Two

Create simple project.