On Wed, Sep 14, 2022 at 08:02:37AM -0400, Owen Hilyard wrote:
> On Wed, Sep 14, 2022 at 3:46 AM Bruce Richardson
> <[1]bruce.richard...@intel.com> wrote:
> > On Fri, Jul 29, 2022 at 10:55:46AM +0000, Juraj Linkeš wrote:
> > > The class adds logging and history records to existing
> pexpect
> > methods.
> > >
> > > Signed-off-by: Owen Hilyard <[1][2]ohily...@iol.unh.edu>
> > > Signed-off-by: Juraj Linkeš <juraj.lin...@pantheon.tech>
> > > ---
> > > dts/framework/ssh_connection.py | 70
> > +++++++++++++++++++++++++++++++++
> > > 1 file changed, 70 insertions(+)
> > > create mode 100644 dts/framework/ssh_connection.py
> > >
> >
> > One comment inline below.
> >
> > /Bruce
> >
> > Two questions on this function:
> >
> > * Is the getattr() check not equivalent to "if self.logger:"?
> >
> > It is. I missed it when looking over this code. I know that
> this close
> > function can run in a context where it loses the ability to
> make system
> > calls (an exit hook), but that doesn't matter for this as far
> as I
> > know.
> >
> > * Why the check for a non-none logger in this function, when
> other
> >
> > functions above always seem to call the logger directly
> without
> > checking?
> >
> > "close" can be called before "init_log" if the program crashes
> early
> > enough, so this is avoiding calling a function on a null
> object. No
> > other function can have that issue because by the time control
> is
> > returned to the user the logger is properly initalized. This is
> > especially important because an early failure in the community
> lab will
> > only be able to use logs to figure out what happened.
> >
> I'm a little confused by that explanation - can you perhaps clarify?
> This
> close function in part of an class, and the logger member is
> assigned its value
> in the constructor for that class, so how is it possible to call
> obj.close() before obj has been constructed?
>
> Due to how we are forced to add the hooks to flush logger buffers to
> disk before shutdown, even in the case of failures or SIGTERM, close
> can run WHILE the constructor is in the middle of executing. All of the
> resources except for the logger can be freed by python, but the logger
> requires special handling, so we check if it is null and then flush the
> buffers to disk if it is not. The actual logger object is only assigned
> to self.logger after it is completely initalized, so if it's not null,
> then it is in a good state and can be safely flushed. Here's a sequence
> of events that would lead to that check being needed:
> 1. Start constructing logging handle
> 2. Execute first line of the constructor
> 3. SIGTERM
> self.logger == None, since the entire world stops and moves to the
> cleanup functions registered with the interpreter. If we don't do this
> check, then the cleanup context crashes in this case.
>
Ack.
Thanks for the clear explanation, having the check now makes sense.
