Initial Fix for Log Files Issue #126
Conversation
|
|
||
| def close_logfile(filename): | ||
| # remove the file from the dict storing open log files and close it | ||
| files.pop(filename).close() |
There was a problem hiding this comment.
There should be some check that the filename exists in our open files before we attempt to close it. Not sure if we should throw an exception if it doesn't exist or just log a warning.
|
|
||
| async def process_ws_message_through_pipeline(): | ||
| '''Prepare each event we receive for processing | ||
| ''' | ||
| events = process_message_from_ws() | ||
| # NOTE: EVENT LOGS ARE USED HERE |
There was a problem hiding this comment.
Remove these notes. They don't make much sense outside of the context of this pull request.
| @@ -190,8 +190,7 @@ async def handler(request, client_event): | |||
| json.dumps(event, sort_keys=True), | |||
| filename, preencoded=True, timestamp=True) | |||
| await pipeline(event) | |||
|
|
|||
| return handler | |||
| return handler, filename | |||
There was a problem hiding this comment.
One downside of this approach is that we have this filename being passed around. The handler is also being passed around, but the fewer items like this the better (less to things to keep track of). Its better to keep all the event handler logic in one place rather than spread out.
One possibility for this is adding a subsequent function to the handler object that takes care of closing, so:
event_handler = handle_incoming_client_event(...)
event_handler(request, event) # normal behavior as it is now
event_handler.close() # new functionality that closes the fileI would be interest in @pmitros 's opinion on this implementation before diving head first into implementing this. What you currently have may be good enough for now.
To implement the functionality, feel free to take a gander at how the KVS is implemented on the system. You can call kvs.KVS() and get the default kvs or use kvs.KVS.memoization() to fetch a specific kvs.
There was a problem hiding this comment.
@bradley-erickson I'm okay with .close(), but I'm wondering if it's necessary.
Have a look at: https://docs.python.org/3.6/library/weakref.html#weakref.finalize
|
I made another commit fixing two of the issues:
|
| old_file = files.pop(filename) | ||
| if old_file is None: | ||
| raise KeyError(f"Tried to remove log file {old_file} but it was not found") | ||
| else: |
There was a problem hiding this comment.
The else statement here is unneeded. You can just do:
if old_file is None:
raise KeyError(...)
old_file.close()If the error gets raised then the last line will not be ran, so need nesting it under an else.
Initial Fix
Created an initial fix for the log file issue that works locally. I added a function
close_logfilethat closes log files and removes them from the system's list of open log files. This function is called when the websocket connection associated with those log files is closed.For the files ending in
.study.log(which are created inevent_decoder_and_logger, this fix works well, as the log closing mechanism is simply included in the same function where the filename is generated.However, for the files just ending in
.log(which are created inhandle_incoming_client_event), it is not ideal because it requires returning a filename from that function and closing the log file outside of the function (the functionhandle_incoming_client_eventcannot determine whether the events have stopped being received, and therefore does not know when to close the log file).This is because while
event_decoder_and_loggerreturns a function that handles an iterable of events, and we can simply close the log file once there are no events remaining (e.g. the socket is closed),handle_incoming_client_eventreturns a handler that is called each time an event is received. This means that if we implemented something similar to withevent_decoder_and_loggerinhandle_incoming_client_event, the log file would be closed after every single event.I tried refactoring
handle_incoming_client_eventto be more similar toevent_decoder_and_logger, returning a function that handles many events, rather than just one event, but this would require changing the code structure significantly.Note: this addresses issue #106 on the ArgLab fork of the repo.