[ENH]: Adding close() method to clients

[tazarov]

Refs: #1756, Also (https://discord.com/channels/1073293645303795742/1209706648243929128)

Description of changes

Summarize the changes made by this PR.

• New functionality
  • Ability to close persistent/HttpClients

Test plan

How are these changes tested?

• Tests pass locally with pytest for python

Documentation Changes

TBD

[github-actions]

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

• Can you think of any use case in which the code does not behave as intended? Have they been tested?
• Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
• If appropriate, are there adequate property based tests?
• If appropriate, are there adequate unit tests?
• Should any logging, debugging, tracing information be added or removed?
• Are error messages user-friendly?
• Have all documentation changes needed been made?
• Have all non-obvious changes been commented?

System Compatibility

• Are there any potential impacts on other parts of the system or backward compatibility?
• Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

• Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

[beggers]

I don't understand the use-case here. Why would someone want to shut down their system without just destroying the object?

[tazarov]

@beggers, there are two issues that users have reported:

• Connections being left in CLOSE_WAIT state, which should be solved by closing the requests session object - https://discord.com/channels/1073293645303795742/1074711446589542552/1217786710851719199
• Their applications failing to shutdown properly due to open resources (from persistent client), which was the primary reason I created the PR - https://discord.com/channels/1073293645303795742/1209706648243929128

While close() might not be the silver bullet here, allowing resources to be freed without making assumptions about how users use Chroma can be beneficial.

[beggers]

My main concern here is that this puts Clients and Servers into a weird and inconsistent state where they can't do anything. Could we instead call this close_connections() and have it kill open network connections?

[tazarov]

For HTTP/Cloud clients, that makes sense; the intent is clear. What are your thoughts about PersistentClient?

[beggers]

PersistentClient doesn't make any network connections IIUC. So it should be a no-op and probably raise an error explaining.

[tazarov]

Actually, the issue with PersistentClient (see above) is that it keeps open files - the SQLite3 db (at least one handle), 4 handles for each open index. The problem for some apps is that they rely on being able to clean up resources e.g. open files.

One challenge I see with implementing close_connections() is that we expose ServerAPI for all clients, meaning that if the abstract method is added at ServerAPI or any of the inherited interfaces, we'll have to implement it (even if no-op) for all clients. We can potentially create a wrapper interface that is specific to the client type and exposes the appropriate methods for resource clean-up. Wdyt?

[HammadB]

This makes sense to me, whats the UX for

• double close
• and use after close

[ArslanSaleem]

@tazarov when this PR is expected to be merged, is anything left in this PR?

[tazarov]

Closing in favor of #2581

[jeffchuber]

@tazarov i think you linked the wrong issue here?

[tazarov]

@jeffchuber, updated to #2581