ArangoDB-Community / arangodb-net-standard

A consistent, comprehensive, minimal interface to enable .NET applications to use the complete range of features exposed by the ArangoDB REST API.
Apache License 2.0
76 stars 30 forks source link

Dispose fix #457

Closed iwedaz closed 10 months ago

iwedaz commented 1 year ago
  1. Don't dispose external resources like HttpClient. They can be injected and controlled by DI system.
  2. Don't force IArangoDBClient interface inherit IDisposable. It doesn't contain anything Disposable.
  3. HttpApiClientResponse is internal resource manager class
rossmills99 commented 1 year ago

I think there's a principle that says "don't dispose stuff you don't own" and so I'm tending to agree that HttpApiTransport should not be disposing the inner HttpClient (although there are static methods used to create HttpApiTransport that do create the HttpClient themselves - grey area?).

However it's hard to know if changing the disposal behaviour might cause problems to existing code that expects it. Perhaps we can introduce some configuration that allows you to "turn off" the disposal of HttpClient. Possibly add an optional parameter for HttpApiTransport constructor:

public HttpApiTransport(HttpClient client, HttpContentType contentType, bool suppressInnerHttpClientDisposal = false)

It can default to false so it does not affect any existing client code. For convenience, we could add the same optional parameter on the static factory methods.

@DiscoPYF would such an approach seem reasonable to you?

DiscoPYF commented 1 year ago

Thanks @rossmills99 👍 From our discussion, I understand better where the changes are coming from in relation to dependency lifetime management with DI frameworks.

This is a good compromise. That way there is no impact for callers that currently use the existing behaviour. I find it best to minimize the amount of breaking changes if possible, even between major releases to not create too much barrier to updating.

@iwedaz Would you be ok updating the pull request or opening a new one with this alternative proposal?

rossmills99 commented 10 months ago

Because #481 just got merged, I'm closing out this PR and associated issue as it will now be possible to prevent disposal using an optional argument in the constructor.