Skip to content

Commit

Permalink
Handle rotation failure in a better way
Browse files Browse the repository at this point in the history 
Previously it would just be a silent failure.

Now, if we really can't open a new file after log rotation, then fall
back to stdErr so that

* user at least has a chance to detect the problem
* we always have a valid OutputStream to log to

Also fix tests which had assumptions which no longer hold (the
OutputStream is wrapped so it never changes from the view of the user)
  • Loading branch information
@spacecowboy
spacecowboy committed Feb 22, 2017
1 parent 2c0e998 commit 2f7767f
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 156 deletions.
7 changes: 3 additions & 4 deletions community/kernel/src/main/java/org/neo4j/kernel/impl/logging/StoreLogService.java
View file
Expand Up @@ -160,23 +160,22 @@ private StoreLogService( LogProvider userLogProvider,
rotationExecutor, new RotatingFileOutputStreamSupplier.RotationListener() rotationExecutor, new RotatingFileOutputStreamSupplier.RotationListener()
{ {
@Override @Override
public void outputFileCreated( OutputStream newStream, OutputStream oldStream ) public void outputFileCreated( OutputStream newStream )
{ {
FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( newStream ); FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( newStream );
logProvider.getLog( StoreLogService.class ).info( "Opened new internal log file" ); logProvider.getLog( StoreLogService.class ).info( "Opened new internal log file" );
rotationListener.accept( logProvider ); rotationListener.accept( logProvider );
} }


@Override @Override
public void rotationCompleted( OutputStream newStream, OutputStream oldStream ) public void rotationCompleted( OutputStream newStream )
{ {
FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( newStream ); FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( newStream );
logProvider.getLog( StoreLogService.class ).info( "Rotated internal log file" ); logProvider.getLog( StoreLogService.class ).info( "Rotated internal log file" );
} }


@Override @Override
public void rotationError( @SuppressWarnings( "unused" ) Exception e, public void rotationError( Exception e, OutputStream outStream )
@SuppressWarnings( "unused" ) OutputStream outStream )
{ {
FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( outStream ); FormattedLogProvider logProvider = internalLogBuilder.toOutputStream( outStream );
logProvider.getLog( StoreLogService.class ).info( "Rotation of internal log file failed:", e ); logProvider.getLog( StoreLogService.class ).info( "Rotation of internal log file failed:", e );
Expand Down
112 changes: 71 additions & 41 deletions community/logging/src/main/java/org/neo4j/logging/RotatingFileOutputStreamSupplier.java
View file
Expand Up @@ -52,15 +52,15 @@ public class RotatingFileOutputStreamSupplier implements Supplier<OutputStream>,
*/ */
public static class RotationListener public static class RotationListener
{ {
public void outputFileCreated( OutputStream newStream, OutputStream oldStream ) public void outputFileCreated( OutputStream out )
{ {
} }


public void rotationCompleted( OutputStream newStream, OutputStream oldStream ) public void rotationCompleted( OutputStream out )
{ {
} }


public void rotationError( @SuppressWarnings("unused") Exception e, @SuppressWarnings("unused") OutputStream out ) public void rotationError( Exception e, OutputStream out )
{ {
} }
} }
Expand All @@ -82,13 +82,23 @@ public void rotationError( @SuppressWarnings("unused") Exception e, @SuppressWar
private final AtomicReference<OutputStream> outRef = new AtomicReference<>(); private final AtomicReference<OutputStream> outRef = new AtomicReference<>();
private final List<WeakReference<OutputStream>> archivedStreams = new LinkedList<>(); private final List<WeakReference<OutputStream>> archivedStreams = new LinkedList<>();


// Used only in case no new output file can be created during rotation
private static final OutputStream stdErrStream = new OutputStream()
{
@Override
public void write( int i ) throws IOException
{
System.err.write( i );
}
};

/** /**
* @param fileSystem The filesystem to use * @param fileSystem The filesystem to use
* @param outputFile The file that the latest {@link OutputStream} should output to * @param outputFile The file that the latest {@link OutputStream} should output to
* @param rotationThresholdBytes The size above which the file should be rotated * @param rotationThresholdBytes The size above which the file should be rotated
* @param rotationDelay The minimum time (ms) after last rotation before the file may be rotated again * @param rotationDelay The minimum time (ms) after last rotation before the file may be rotated again
* @param maxArchives The maximum number of archived output files to keep * @param maxArchives The maximum number of archived output files to keep
* @param rotationExecutor An {@link Executor} for performing the rotation * @param rotationExecutor An {@link Executor} for performing the rotation
* @throws IOException If the output file cannot be created * @throws IOException If the output file cannot be created
*/ */
public RotatingFileOutputStreamSupplier( FileSystemAbstraction fileSystem, File outputFile, public RotatingFileOutputStreamSupplier( FileSystemAbstraction fileSystem, File outputFile,
Expand All @@ -100,13 +110,14 @@ public RotatingFileOutputStreamSupplier( FileSystemAbstraction fileSystem, File
} }


/** /**
* @param fileSystem The filesystem to use * @param fileSystem The filesystem to use
* @param outputFile The file that the latest {@link OutputStream} should output to * @param outputFile The file that the latest {@link OutputStream} should output to
* @param rotationThresholdBytes The size above which the file should be rotated * @param rotationThresholdBytes The size above which the file should be rotated
* @param rotationDelay The minimum time (ms) after last rotation before the file may be rotated again * @param rotationDelay The minimum time (ms) after last rotation before the file may be rotated again
* @param maxArchives The maximum number of archived output files to keep * @param maxArchives The maximum number of archived output files to keep
* @param rotationExecutor An {@link Executor} for performing the rotation * @param rotationExecutor An {@link Executor} for performing the rotation
* @param rotationListener A {@link org.neo4j.logging.RotatingFileOutputStreamSupplier.RotationListener} that can observe the rotation process and be notified of errors * @param rotationListener A {@link org.neo4j.logging.RotatingFileOutputStreamSupplier.RotationListener} that can
* observe the rotation process and be notified of errors
* @throws IOException If the output file cannot be created * @throws IOException If the output file cannot be created
*/ */
public RotatingFileOutputStreamSupplier( FileSystemAbstraction fileSystem, File outputFile, public RotatingFileOutputStreamSupplier( FileSystemAbstraction fileSystem, File outputFile,
Expand Down Expand Up @@ -179,7 +190,9 @@ public OutputStream get()
{ {
if ( !closed.get() && !rotating.get() ) if ( !closed.get() && !rotating.get() )
{ {
if ( rotationDelayExceeded() && rotationThresholdExceeded() ) // In case output file doesn't exist, call rotate so that it gets created
if ( rotationDelayExceeded() && rotationThresholdExceeded() ||
!fileSystem.fileExists( outputFile ) )
{ {
rotate(); rotate();
} }
Expand Down Expand Up @@ -236,51 +249,67 @@ private void rotate()
synchronized ( outRef ) synchronized ( outRef )
{ {
OutputStream oldStream = outRef.get(); OutputStream oldStream = outRef.get();
OutputStream newStream;


// Must close file prior to doing any operations on it or else it won't work on Windows
try try
{ {
oldStream.flush(); // Must close file prior to doing any operations on it or else it won't work on Windows
oldStream.close(); try
} {
catch ( Exception e ) oldStream.flush();
{ oldStream.close();
rotationListener.rotationError( e, outRef.get() ); }
rotating.set( false ); catch ( Exception e )
return; {
} rotationListener.rotationError( e, streamWrapper );
rotating.set( false );
return;
}


OutputStream newStream; try
try {
{ if ( fileSystem.fileExists( outputFile ) )
if ( fileSystem.fileExists( outputFile ) ) {
shiftArchivedOutputFiles();
fileSystem.renameFile( outputFile, archivedOutputFile( 1 ) );
}
}
catch ( Exception e )
{ {
shiftArchivedOutputFiles(); rotationListener.rotationError( e, streamWrapper );
fileSystem.renameFile( outputFile, archivedOutputFile( 1 ) ); rotating.set( false );
return;
} }
newStream = openOutputFile();
} }
catch ( Exception e ) finally
{ {
rotationListener.rotationError( e, outRef.get() ); try
rotating.set( false ); {
return; newStream = openOutputFile();
}
catch ( IOException e )
{
newStream = stdErrStream;
rotationListener.rotationError( e, streamWrapper );
rotating.set( false );
}
} }


rotationListener.outputFileCreated( newStream, oldStream );

if ( !closed.get() ) if ( !closed.get() )
{ {
outRef.set( newStream ); outRef.set( newStream );
removeCollectedReferences( archivedStreams ); removeCollectedReferences( archivedStreams );
archivedStreams.add( new WeakReference<>( oldStream ) ); archivedStreams.add( new WeakReference<>( oldStream ) );
} }


rotationListener.outputFileCreated( streamWrapper );

if ( rotationDelay > 0 ) if ( rotationDelay > 0 )
{ {
earliestRotationTimeRef.set( currentTimeSupplier.getAsLong() + rotationDelay ); earliestRotationTimeRef.set( currentTimeSupplier.getAsLong() + rotationDelay );
} }
rotationListener.rotationCompleted( newStream, oldStream ); rotationListener.rotationCompleted( streamWrapper );
rotating.set( false ); rotating.set( false );
} }
}; };
Expand All @@ -291,7 +320,7 @@ private void rotate()
} }
catch ( Exception e ) catch ( Exception e )
{ {
rotationListener.rotationError( e, outRef.get() ); rotationListener.rotationError( e, streamWrapper );
rotating.set( false ); rotating.set( false );
} }
} }
Expand All @@ -309,7 +338,8 @@ private void shiftArchivedOutputFiles() throws IOException
if ( i >= maxArchives ) if ( i >= maxArchives )
{ {
fileSystem.deleteFile( archive ); fileSystem.deleteFile( archive );
} else }
else
{ {
fileSystem.renameFile( archive, archivedOutputFile( i + 1 ) ); fileSystem.renameFile( archive, archivedOutputFile( i + 1 ) );
} }
Expand Down

0 comments on commit 2f7767f

Please sign in to comment.