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

Rethink connection lifecycle and 'eager' disconnecting #1805

Open
bitprophet opened this Issue Jun 19, 2018 · 1 comment

Comments

Projects
None yet
1 participant
@bitprophet
Member

bitprophet commented Jun 19, 2018

Background

v1:

  • kept a (partly-documented) connection cache around, with the intent to prevent unnecessary reconnections to servers that might be spoken to by multiple tasks
    • though it was also a requirement forced on us by the global state design - with no explicit objects modeling those connections, keeping that cache was the only way to avoid creating a brand new end-to-end connection on every single run().
  • A disconnect_all function iterates the cache and explicitly closes the client objects / removes them, and it's called at end of session
  • We later added an opt-in eagerly_disconnect flag to force calling disconnect_all after every task, because advanced users found that lots of only-spoken-to-once servers can hog resources (think running out of file handles for sockets due to a typically-lowish ulimit, for example)

v2:

  • did not implement a cache, because it did implement explicit per-connection objects with an explicit lifecycle (up to and including support for use as contextmanagers)
  • should not need any explicit eager disconnecting, since each task invocation gets its own connection object, and connections ought to be getting GC'd when they go out of scope
  • may need double-checking of that, and updating either to ensure Connection does the right thing when GC'd, or to explicitly .close (by the Executor, most likely) at end of task and/or session
    • If that latter, then there's real potential work/discussion to be had, around whether we should close at end of task, session, both, configurability, etc

TODO

  • Double check garbage-collection behavior of Connection (and/or SSHClient) generally, re: whether it correctly closes socket when exits scope
  • Regardless of above, double check whether Connections do exit scope early or if they hang out until end of session.
    • AFAIK, it should be the former, since they only exist as variables within a given call of one or two Executor methods, and should thus disappear each time the Executor cycles through the tasks or permutations of tasks being invoked.
    • But thats' why it's a double check...
  • If they are not being GC'd as early as we'd like, identify why and whether it's a simple fix, probably release as a bugfix
  • Either way, document clearly the intended/current behavior, and identify whether there are reasonable opportunities for user configuration. If those are implemented, put it out as a feature.
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Jul 16, 2018

Member

Kinda-sorta related: #1473 - insofar as the more we explicitly shut down connections on users' behalf, the less likely that end of session hang may be?

Member

bitprophet commented Jul 16, 2018

Kinda-sorta related: #1473 - insofar as the more we explicitly shut down connections on users' behalf, the less likely that end of session hang may be?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment