Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpMessageHandler Passed To Constructor Of FhirClient Gets Disposed When FhirClient Is Disposed #2029

Closed
AMehlem opened this issue Mar 30, 2022 · 5 comments · Fixed by #2046

Comments

@AMehlem
Copy link

AMehlem commented Mar 30, 2022

Describe the bug
We are using the firely-net-sdk in a project and are experiencing port exhaustion when the server-web-api using the sdk is under high load. To deal with the issue, I tried to reuse the HttpMessageHandler that gets passed into the constructor of the FhirClient. However, the HttpMessageHandler gets disposed with the HttpClientRequester, that is created in the FhirClient's constructor.

Expected behavior
If I am not mistaken, a class instance passed into a constructor should in general not be disposed by the class using it. The BaseFhirClient should therefore only dispose the HttpMessageHandler if the class BaseFhirClient or the FhirClient created it. If the HttpMessageHandler was passed as a constructor parameter, the handler should not be disposed with the BaseFhirClient.

Version used:

  • FHIR Version: R4, maybe other versions are also affected
  • Version: 3.8.0

Kind regards,
Ashwani Mehlem

@brianpos
Copy link
Collaborator

May also be related to #2036

@brianpos
Copy link
Collaborator

Have confirmed this behaviour, and have a simple fix for it.

brianpos added a commit to brianpos/fhir-net-api that referenced this issue Apr 11, 2022
@brianpos
Copy link
Collaborator

Along with this common module version
brianpos/firely-net-common@69eb790

@marcovisserFurore
Copy link
Member

Good idea to use the boolean disposeHandler of the HttpClient constructor. It saves us a flag in the FhirClient to remember not dispose the injected handler.

We will create a PR for this.

@AMehlem
Copy link
Author

AMehlem commented Apr 11, 2022

Thanks @brianpos, your fix solves the problem with the port exhaustion as the message handler can then be reused.

As a side note: It would be even easier if the constructor of the class FhirClient would take an HttpClient or an IHttpClientFactory as a parameter instead of an HttpMessageHandler, as this would allow using the HttpClientFactory-Pattern (see https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-6.0) already mentioned in issue #2036.
The main benefit of this is that the HttpClientFactory handles the lifetime of the message handlers, which avoids DNS-Issues (which can occur when the message handlers live too long) as well as port exhaustion and performance problems (which may occur when too many message handlers are created in a short period of time).

@AMehlem AMehlem closed this as completed Apr 11, 2022
brianpos added a commit that referenced this issue Apr 11, 2022
…sion that includes the default parameter for the disposeHandler (even though we pass it throuth)
marcovisserFurore pushed a commit that referenced this issue Apr 20, 2022
…posed when it is done with it.

# Conflicts:
#	src/Hl7.Fhir.Core.Tests/Rest/FhirClientTests.cs
marcovisserFurore pushed a commit that referenced this issue Apr 20, 2022
…sion that includes the default parameter for the disposeHandler (even though we pass it throuth)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants