FhirClient SearchAsync not thread safe

[oldefezziwig]

I am able to reproduce this issue pretty reliably, using both Hl7.Fhir.STU3 0.96.0.0 and 1.3.0 (When we run this, one of those Assert.IsTrue()'s eventually fail for us). Here is my test code (uses NUnit 3.10.0). There are some helper methods and whatnot that we've implemented (i.e. the "retryPolicyFactory" just supports the ability to retry any operation a few times if they fail for any reason, the _patientRootOid could be just any old OID, and the FhirClient here is actually a wrapper because we need to inject an OAUTH token header for each request), but none of that should have a bearing on the results of the test.

Using _numParallelThreads = 15 for the purposes of this test.

Also, when we spin up multiple FhirClients (or possibly create a new one at the beginning of every iteration of the Parallel.ForEach), this issue does not occur. So is that a valid solution or workaround (i.e. to ensure each separately running Task is using its own FhirClient instance)?
We were under the impression that a single FhirClient instance should be used by multiple concurrent Tasks, but per this test that does not appear to be the case.

If anyone is able to reproduce this issue, please comment back here as this is becoming a real operational headache for us. Thanks!

`[Test]
public async System.Threading.Tasks.Task TestFhirClientCallsInParallel()

{
  // Generate patients
  var patients = new List<Patient>();
  for (int i = 1; i < 2000; i++)
  {
    patients.Add(new Patient
    {
      Name = new List<HumanName>
      {
        new HumanName
        {
          Family = $"Doe{i}",
          Given = new List<string>
          {
            $"John{i}"
          }
        }
      },
      Identifier = new List<Identifier>
      {
        new Identifier(_patientRootOid, i.ToString())
        {
          Use = Identifier.IdentifierUse.Official,
          Type = new CodeableConcept("http://hl7.org/fhir/v2/0203", "MR")
        }
      }
    });
  }


  await System.Threading.Tasks.Task.Factory.StartNew(() =>
    Parallel.ForEach(patients, new ParallelOptions
    {
      MaxDegreeOfParallelism = _numParallelThreads
    }, async patient =>
    {
      // Use single FhirClient (instantiated as a private var)
      var client =  _fhirCient;


      Console.WriteLine($"Processing Patient {patient.Identifier.FirstOrDefault()?.Value}");
      // create patient
      var createdPatient = await client.CreateAsync(patient, _retryPolicyFactory.CreateCatalystRetryPolicy());
      Assert.NotNull(createdPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(createdPatient.Identifier));


      // query for patient
      var queriedPatientBundle = await client.SearchAsync<Patient>(new[] {$"identifier={patient.Identifier.FirstOrDefault()?.System}|{patient.Identifier.FirstOrDefault()?.Value}"}, _retryPolicyFactory.CreateCatalystRetryPolicy());
      var queriedPatient = queriedPatientBundle?.Entry?.ByResourceType<Patient>()?.FirstOrDefault();
      Assert.NotNull(queriedPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(queriedPatient.Identifier), $"First search for identifier {JsonConvert.SerializeObject(patient.Identifier)} but returned {JsonConvert.SerializeObject(queriedPatient.Identifier)}");


      // update patient
      queriedPatient.Name.FirstOrDefault()?.GivenElement.Add(new FhirString("Q"));
      var updatedPatient = await client.UpdateAsync(queriedPatient, _retryPolicyFactory.CreateCatalystRetryPolicy());
      Assert.NotNull(updatedPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(updatedPatient.Identifier));


      // query for patient (2nd)
      queriedPatientBundle = await client.SearchAsync<Patient>(new[] {$"identifier={patient.Identifier.FirstOrDefault()?.System}|{patient.Identifier.FirstOrDefault()?.Value}"}, _retryPolicyFactory.CreateCatalystRetryPolicy());
      queriedPatient = queriedPatientBundle?.Entry?.ByResourceType<Patient>()?.FirstOrDefault();
      Assert.NotNull(queriedPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(queriedPatient.Identifier), $"Second search for identifier {JsonConvert.SerializeObject(patient.Identifier)} but returned {JsonConvert.SerializeObject(queriedPatient.Identifier)}");


      // update patient (2nd)
      queriedPatient.Gender = AdministrativeGender.Male;
      updatedPatient = await client.UpdateAsync(queriedPatient, _retryPolicyFactory.CreateCatalystRetryPolicy());
      Assert.NotNull(updatedPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(updatedPatient.Identifier));


      // query for patient
      queriedPatientBundle = await client.SearchAsync<Patient>(new[] {$"identifier={patient.Identifier.FirstOrDefault()?.System}|{patient.Identifier.FirstOrDefault()?.Value}"}, _retryPolicyFactory.CreateCatalystRetryPolicy());
      queriedPatient = queriedPatientBundle?.Entry?.ByResourceType<Patient>()?.FirstOrDefault();
      Assert.NotNull(queriedPatient);
      Assert.IsTrue(patient.Identifier.IsExactly(queriedPatient.Identifier), $"Third search for identifier {JsonConvert.SerializeObject(patient.Identifier)} but returned {JsonConvert.SerializeObject(queriedPatient.Identifier)}");


      // delete patient
      await client.DeleteAsync(queriedPatient, _retryPolicyFactory.CreateCatalystRetryPolicy());
    })
  );
}`

[BradBarnich]

FhirClient is not thread-safe. It holds on to state from the current operation.

[ewoutkramer]

That's right. By design, most classes (just like the .NET framework by the way) are not thread-safe. The FhirClient is pretty lightweight to construct and indeed keeps state per request (like the result of the last request).

@gstoian is re-architecting a new FhirClient based on @BradBarnich's previous work - so this would be a good moment to discuss whether the FhirClient should or should not be thread-safe.

[oldefezziwig]

@BradBarnich, @ewoutkramer Thanks for the information. I figured it was intended to be thread safe based on this documentation: https://ewoutkramer.github.io/fhir-net-api/client-setup.html :
You'll create an instance of a client for every server you want to work with.
This seemed to imply that if you were only dealing with a single FHIR server, you would only need a single instance of the client (but doesn't mention multithreading).

In our case, we are only dealing with a single FHIR server, but in some cases need to "multithread" (using async/await and something like Parallel.ForEach with a MaxDegreeOfParallism, not actually dealing with creating Threads ourselves). So if it is ok to essentially create a separate FhirClient per concurrent processing workflow (i.e. for the life of a given Task, that may make multiple requests based on a single quantity of incoming data), then we will just do that for now. Thanks again!!

[ewoutkramer]

Good catch, I can see how that causes confusion. @mbaltus would you be able to change that into something that does not suggest to use a single client per thread?

[ewoutkramer]

@mbaltus - can you change this line.

Also, we need to take the documentation on github.io offline - its wildly outdated.

[ewoutkramer]

Updated the documentation AND redirected the users away from the github.io pages.

[oldefezziwig]

@ewoutkramer I see that you updated the documentation to show the following:

Note: the FhirClient is not thread-safe, so you will need to create one for each thread, if necessary. But don’t worry: creating an instance of a FhirClient is cheap, the connection will not be opened until you start working with it.

Can you verify that this is still the case in the new 2.x version that uses HttpClient? Or is that new version threadsafe and the proper usage when used in parallel (using the async framework) is to just create a single FhirClient per endpoint even if it may be used in parallel (like in https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.parallel.foreach?view=net-5.0 for example)?

[ewoutkramer]

Yes, option b. We want to keep the options open that we cache stuff or do non-threadsafe things in our implementations.

There are only a few classes (mostly around caching) in the SDK that are threadsafe. So we could do it if there's a strong usecase to have a single instance. But I haven't heard one so far.

[oldefezziwig]

@ewoutkramer sorry I'm still confused about what we are supposed to do here. We need to process data in parallel using a mechanism similar to the one shown in my first message back in #1110 (comment)

We now have realized that we need to use the 2.x fhir-net-api (and we do like that FhirClient now uses HttpClient under the hood), so we are in a bit of a bind as to how to use it in parallel. If you look at the test code I posted when I created this ticket, which one of the following should be the suggested usage for FhirClient 2.0?

1. As-is in the code above, meaning _fhirClient is a singleton hitting the same endpoint (recall that this approach did not work for 1.x.. the SearchAsync on Patient would occasionally return the wrong Patient)
2. Instead of the line var client = _fhirCient; it would be var client = new FhirClient("http://server.fire.ly"); created for every iteration of the parallel loop (this is essentially what we had to do to get 1.x to work, but we have read a lot of warnings about bad side effects of instantiating multiple HttpClients for the same endpoint in the same process: https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/). Note that we didn't test this approach yet using FhirClient 2.0, but were concerned about these warnings and were under the impression that anytime you used HttpClient, you should just create one per endpoint in a given process, so we are not sure what to do here.

UPDATE:.. A 3rd option we are kicking around is generating a pool of FhirClients whose size would be equal to the "max parallelism" that would be allocated and assigned (blocking until one becomes available), and so perhaps it would avoid the thread safety issues inherent in using a single FhirClient, but also prevent issues described in the above article with instantiating umpteen million FhirClients (each of which creates a new HttpClient)

[ewoutkramer]

I was going to say:

So if it is ok to essentially create a separate FhirClient per concurrent processing workflow (i.e. for the life of a given Task, that may make multiple requests based on a single quantity of incoming data), then we will just do that for now. Thanks again!!

This is your solution. But then I read:

we have read a lot of warnings about bad side effects of instantiating multiple HttpClients for the same endpoint in the same process

Well, I guess that is right. Because looking at the code, if you'd fire a large number of parrallel request you'd literally be swamping the target server and (on a lower level) the available interrupts on your own machine. The target server's owner (unless that is you), might not really enjoy being targeted this way!

So, the more advanced approach on having a pool seem to make sense, but that of course limits your parrallelism.

[stonstad]

I think this topic may need to be revisited. Support for thread safety is a fundamental feature of a RESTful client.

[ewoutkramer]

What would be needed? Getting a fresh (stateful) client out of a pool of clients for the same endpoint with the same authentication settings like discussed above? I personally have never needed thread safety, instead have created a new instance of the client, so I'd like to get a better feel of where the client is deficient currently.

[ewoutkramer]

I guess what we need to do is centered around these best-practices?

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0

[ewoutkramer]

#2036 - according to @brianpos we should be alright in this regard.

[oldefezziwig]

@ewoutkramer @brianpos so are you saying that if we use the version with feature #2055 that we should now be safe to use HttpClientFactory to create a single HttpClient (that we maintain and dispose ourselves) passed into new instances of FhirClient constructor (as opposed to a pool of clients or a single FhirClient, both approaches as described above each have their own drawbacks)? Put another way, this gets around the issues described in https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/ but that the reused HttpClient should itself be threadsafe?