Description
Describe the bug
DicomPixelData's GetFrame is not thread safe when a DicomFile is opened from a stream. I believe this is due to a race condition in StreamByteBuffer between it setting position and reading from the stream.
This results in the wrong image frames being returned.
I think the same issue means it's possible to have DicomFiles fail to read any tags properly if you have the same stream in use via DicomPixelData and a different DicomFile instance.
We're doing this to get at the raw pixel data in order to use a much faster jpeg decoder in the common case.
Discussion:
I think it's intended that this issue doesn't occur; DicomImage makes an effort to prevent this (or similar concurrency) issues by locking on itself, but that's not quite enough.
I'm not entirely sure how to safely use seekable streams in parallel. Would it be possible to resolve this by having StreamByteBuffer lock on the stream itself? Or is there no guarantee the stream instance would be the same?
That would still have problems if the position was changed outside fo-dicom, but that's understandable.
To Reproduce
The following test reproduces within seconds to minutes on 5.0.3 and latest dev, however as it's a race condition it's hard to guarantee!
namespace FellowOakDicom.Tests.Bugs
{
public class ByteBufferBug
{
[Fact]
public void ConcurrentGetFrameFromAStreamBasedDicomFile_ShouldWork()
{
// Arrange
var inputFile = new FileInfo(TestData.Resolve("GH645.dcm"));
using var fs = File.OpenRead(inputFile.FullName);
using var ms = new MemoryStream();
fs.CopyTo(ms);
ms.Seek(0, SeekOrigin.Begin);
var streamByteDicomFile = DicomFile.Open(ms);
var referencePixelData = DicomPixelData.Create(streamByteDicomFile.Dataset);
var referenceBytes = Enumerable.Range(0, referencePixelData.NumberOfFrames).Select(x => referencePixelData.GetFrame(x).Data)
.ToArray();
// Act
var streamBytePixelData = DicomPixelData.Create(streamByteDicomFile.Dataset.Clone());
Parallel.For(0, 10000, i =>
{
var index = i % streamBytePixelData.NumberOfFrames;
var frame = streamBytePixelData.GetFrame(index);
Assert.Equal(referenceBytes[index], frame.Data);
});
}
}
}I did most of my investigation on 5.0.3, and I do see some changes in the IO namespace in latest dev, but the test still fails, so I hope it's the same problem.
Expected behavior
Either:
- The correct image frame is returned
- or an exception is thrown
Environment
Fellow Oak DICOM version: 5.0.3, latest dev
OS: Windows 10, Ubuntu 22.04
Platform: .NET 7.0