Item12359: Func::eachEventSince can leak file descriptors

Foswiki::Func::eachEventSince() calls the Logger eachEventSince function. It opens as many files as required to cover the requested range, and returns an iterator to the caller. There is no path to close the opened files.

• Document that the file handle will be closed when the iterator is destroyed.
• Add a sub DESTROY() to the Foswiki::LineIterator object which cleans up the file handle.

-- GeorgeClark - 19 Jan 2013

Running LoggerTests with CHECKLEAK enabled finds additional issues, Some seem to be internal to Log::Dispatch.

-- GeorgeClark - 19 Jan 2013

I'm not sure this is exactly the right implementation.

Foswiki::LineIterator doesn't open the file (looks like it takes an open filehandle as input), so I don't think it should close it. That breaks encapsulation/modularity.

Rather, the upper level object that does the open() should own doing the close.

If eachEventSince does the open and is directly returning a Foswiki::LineIterator object, it shouldn't.

Instead, it should create its own object - and that object's DESTROY does the close. That's pretty easy if the Logger object inherits from LineIterator.

E.g. (Don't quote me on the exact package names, but to get you started, you want something like this:)

package Foswiki::Logger; # Or whatever contains the code that Func::eachEventSince invokes

sub eachEventSince {
   ...

  open( my $fh, '<', $theLogFileName ) or something. # die, return undef, skip file...
  my $logIt = Foswiki::LogIterator->new($fh);
  $logIt->{logLocked} = eval { flock( $fh, LOCK_SH ) };  # No error in case on non-flockable FS; eval in case flock not supported.

   ...

   return $logIt;
}
 # In the same file is OK here

package Foswiki::LogIterator;

# Internal class for Logfile iterators.

# Inherit all the iterator methods from LineIterator

our @ISA = qw/Foswiki::LineIterator/;

# Object destruction
# Release locks and file

sub DESTROY {
  my $logIt = shift;

  flock( $logIt->{handle}, LOCK_UN ) if( delete $logIt->{logLocked} );
  close( $logIt->{handle} ) if( delete $logIt->{handle} );
}

Notes: One might argue for not storing Logger data in the LineIterator hash - it would be purer, but a bit more trouble as you'd need to implement all the LineIterator methods passing the LineIterator handle from your object to each. It's not worth the trouble. Likewise, it's impure to access {handle} from here - you could add an interface method, but it also doesn't seem worth the trouble.

What is important is that the Logger is responsible for the file handle being open (and locked). When it's done with the handle, it knows what to do - and LineIterator doesn't. Locks are one example. (We are locking, right?)

But Logger might want to chmod a file after processing (say in a rotate), or rewind for another pass, or...

So DESTROY is the right idea. But the destructor is, I think, in the wrong place.

(Obviously, this has to be generalized if you have more than one file/iterator.)

-- TimotheLitt - 20 Jan 2013

I think this is done right now. Destructor and open/close and lock/unlock are all in the same files.

-- GeorgeClark - 07 Jan 2014

Commenting is disabled while not logged in