-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Optional keepalive for the http connection #3
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
base: master
Are you sure you want to change the base?
Conversation
Long running connections and strict firewall policies (one can argue that are not RFC compliant but that's besides the point) can incur in connections getting stalled forever. Upon connecting to the OBS backend, every so minutes (5 min) to "keep the line up".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I need to think this through some more, if there could be any multi-threading issues here. The osc
module implements transparent connection reuse these days and multi-threaded access could break things.
oscfs/fs.py
Outdated
|
||
@contextlib.contextmanager | ||
def optional_keepalive(self): | ||
if "--no-urlopen-wrapper" in sys.argv: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why couple this to --no-urlopen-wrapper
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason, I suspected the keepalive would be more impactful in conjunction with the urlopen wrapper but I can take it away.
oscfs/fs.py
Outdated
direct_io=True, | ||
nonempty=True | ||
) | ||
with self.optional_keepalive(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this contextlib approach very much. It is more or less misused here IMHO and it's hard to understand what it is about.
The thread could also be started in init()
, I suppose. Shutdown, if necessary, could happen after FUSE returns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made it so it was an "optional feature" trying to minimize the changes to the main routine by yielding.
Starting the thread on init
indeed looks like a better alternative.
oscfs/fs.py
Outdated
def run(self): | ||
def _keepalive(self): | ||
while self._keepalive: | ||
time.sleep(5 * 60) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ping frequency should be a configurable command line argument
# unallocated file handles | ||
self.m_free_handles = list(range(1024)) | ||
self._setupParser() | ||
self._keepalive_timer = threading.Thread(target=self._keepalive_thread) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the thread is started only if its actually needed, in the init()
method as well.
Currently there is also a race, because the self._keepalive
flag might be unset when the thread enters its loop.
Apart from the one comment I believe this might work now. Can you please address the problem and squash the commits you did so far? I will then make some runtime tests to see if it holds up in practice before I merge this. Thanks! |
Long running connections and strict firewall policies (one can argue that are not RFC compliant but that's besides the point) can incur in connections getting stalled forever.
Upon connecting to the OBS backend, every so minutes (5 min) to "keep the line up" when using a connection pool to interact with the OBS Backend.