[analysis_server] Prevent session log being overwritten on every message

This changes the SessionLogFileSink from using `writeAsStringSync` and overwriting the whole file on each message. It does this by using `openWrite()` and keeping the `IOSink` instead.

This means:

- it uses `dart:io` (we don't have `openWrite`/sink support in the abstraction)
- we need to call `close()` to flush the file during shutdown (there was already a shutdown method on the session logger, but it wasn't used or called, so now it is)

This does mean the tests for this class write to the physical disk, but there are only two of them - most other tests use the in-memory sink (or are just testing the normalizer).

Fixes https://github.com/dart-lang/sdk/issues/63275

Change-Id: I7ef347fc46d8ce3daf30ed2f9965e8921c5c4856
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/501640
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Commit-Queue: Samuel Rawlins <srawlins@google.com>

pkg/analysis_server/integration_test/lsp_server/integration_tests.dart

@@ -9,7 +9,6 @@ import 'dart:io';
 import 'package:analysis_server/lsp_protocol/protocol.dart';
 import 'package:analysis_server/src/lsp/channel/lsp_byte_stream_channel.dart';
 import 'package:analysis_server/src/services/pub/pub_command.dart';
-import 'package:analysis_server/src/session_logger/session_logger.dart';
 import 'package:analyzer/file_system/physical_file_system.dart';
 import 'package:analyzer/instrumentation/instrumentation.dart';
 import 'package:analyzer_plugin/src/utilities/client_uri_converter.dart';
@@ -242,7 +241,6 @@ class LspServerClient {
       inputStream,
       outputStream,
       instrumentationService ?? InstrumentationLogAdapter(PrintableLogger()),
-      sessionLogger: SessionLogger(),
     )..listen(_serverToClient.add);
   }
 

pkg/analysis_server/lib/src/analysis_server.dart

@@ -1169,6 +1169,7 @@ abstract class AnalysisServer {
     await contextManager.dispose();
     await analyticsManager.shutdown();
     await shutdownPerfWitness();
+    await sessionLogger.shutdown();
   }
 
   ResolvedForCompletionResultImpl?

pkg/analysis_server/lib/src/lsp/channel/lsp_byte_stream_channel.dart

@@ -25,7 +25,7 @@ class LspByteStreamServerChannel implements LspServerCommunicationChannel {
   final InstrumentationService _instrumentationService;
 
   /// The session logger.
-  final SessionLogger _sessionLogger;
+  final SessionLogger? _sessionLogger;
 
   /// Completer that will be signalled when the input stream is closed.
   final Completer<void> _closed = Completer();
@@ -37,8 +37,8 @@ class LspByteStreamServerChannel implements LspServerCommunicationChannel {
     this._input,
     this._output,
     this._instrumentationService, {
-    SessionLogger? sessionLogger,
-  }) : _sessionLogger = sessionLogger ?? SessionLogger();
+    this._sessionLogger,
+  });
 
   /// Future that will be completed when the input stream is closed.
   @override
@@ -94,7 +94,7 @@ class LspByteStreamServerChannel implements LspServerCommunicationChannel {
     }
     _instrumentationService.logRequest(data);
     var json = jsonDecode(data) as Map<String, Object?>;
-    _sessionLogger.logMessage(
+    _sessionLogger?.logMessage(
       from: ProcessId.ide,
       to: ProcessId.server,
       message: json,
@@ -129,7 +129,7 @@ class LspByteStreamServerChannel implements LspServerCommunicationChannel {
     _write(utf8EncodedBody);
 
     _instrumentationService.logResponse(jsonEncodedBody);
-    _sessionLogger.logMessage(
+    _sessionLogger?.logMessage(
       from: ProcessId.server,
       to: ProcessId.ide,
       message: json,

pkg/analysis_server/lib/src/server/driver.dart

@@ -343,10 +343,7 @@ class Driver implements ServerStarter {
 
     // Initialize the session logging service.
     var sessionLogFilePath = results.option(sessionLogOption);
-    var sessionLogFile = sessionLogFilePath == null
-        ? null
-        : PhysicalResourceProvider.INSTANCE.getFile(sessionLogFilePath);
-    _sessionLogger = SessionLogger(sessionLogFile: sessionLogFile);
+    _sessionLogger = SessionLogger(filePath: sessionLogFilePath);
     _sessionLogger.normalizer.addReplacementsForPath(
       defaultSdkPath,
       'dartSdkRoot',

pkg/analysis_server/lib/src/session_logger/session_logger.dart

@@ -8,7 +8,6 @@ import 'package:analysis_server/src/session_logger/log_entry.dart';
 import 'package:analysis_server/src/session_logger/log_normalizer.dart';
 import 'package:analysis_server/src/session_logger/process_id.dart';
 import 'package:analysis_server/src/session_logger/session_logger_sink.dart';
-import 'package:analyzer/file_system/file_system.dart';
 
 /// Used to write information about a session to a log.
 class SessionLogger {
@@ -24,12 +23,12 @@ class SessionLogger {
   ///
   /// If [filePath] is non-`null`, it also writes log entries to a file at
   /// [filePath].
-  factory SessionLogger({File? sessionLogFile}) {
+  factory SessionLogger({String? filePath}) {
     var normalizer = LogNormalizer();
     var sink = SessionLoggerInMemorySink(
       maxBufferLength: 1024,
       normalizer: normalizer,
-      sessionLogFile: sessionLogFile,
+      sessionLogFilePath: filePath,
     );
     return SessionLogger._(sink: sink, normalizer: normalizer);
   }
@@ -82,5 +81,7 @@ class SessionLogger {
   }
 
   /// Shuts down the logger.
-  Future<void> shutdown() async {}
+  Future<void> shutdown() async {
+    await sink?.close();
+  }
 }

pkg/analysis_server/lib/src/session_logger/session_logger_sink.dart

@@ -3,29 +3,34 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'dart:convert';
+import 'dart:io' as io;
 
 import 'package:analysis_server/src/session_logger/log_entry.dart';
 import 'package:analysis_server/src/session_logger/log_normalizer.dart';
 import 'package:analysis_server/src/session_logger/process_id.dart';
-import 'package:analyzer/file_system/file_system.dart';
 import 'package:language_server_protocol/protocol_special.dart' show Either2;
 
 /// A sink for a session logger that will write entries to a file.
 final class SessionLoggerFileSink extends SessionLoggerSink {
   /// The sink used to write to the file.
-  final File _file;
+  late final io.IOSink _sink;
 
   @override
   final LogNormalizer _normalizer;
 
   /// Initializes a newly created sink to write to the file at the given
   /// [filePath].
-  SessionLoggerFileSink(this._file, {required this._normalizer});
+  SessionLoggerFileSink(String filePath, {required this._normalizer}) {
+    _sink = io.File(filePath).openWrite();
+  }
+
+  @override
+  Future<void> close() => _sink.close();
 
   @override
   void writeLogEntry(JsonMap entry) {
     var jsonString = _normalizer.normalize(json.encode(entry));
-    _file.writeAsStringSync('$jsonString\n');
+    _sink.writeln(jsonString);
   }
 }
 
@@ -71,11 +76,11 @@ final class SessionLoggerInMemorySink extends SessionLoggerSink {
   SessionLoggerInMemorySink({
     required this.maxBufferLength,
     required LogNormalizer normalizer,
-    File? sessionLogFile,
+    String? sessionLogFilePath,
   }) : _normalizer = normalizer,
-       _nextLogger = sessionLogFile == null
+       _nextLogger = sessionLogFilePath == null
            ? null
-           : SessionLoggerFileSink(sessionLogFile, normalizer: normalizer);
+           : SessionLoggerFileSink(sessionLogFilePath, normalizer: normalizer);
 
   /// Returns a list of the entries that have been captured.
   ///
@@ -209,6 +214,8 @@ sealed class SessionLoggerSink {
   /// The normalizer used to normalize paths in log entries.
   LogNormalizer get _normalizer;
 
+  Future<void> close() async {}
+
   /// Writes the given log [entry] to this sink.
   void writeLogEntry(JsonMap entry);
 

pkg/analysis_server/test/src/session_logger/session_logger_sink_test.dart

@@ -2,11 +2,14 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
+import 'dart:io' as io;
+
 import 'package:analysis_server/src/session_logger/log_normalizer.dart';
 import 'package:analysis_server/src/session_logger/session_logger_sink.dart';
 import 'package:analyzer/file_system/memory_file_system.dart';
+import 'package:analyzer/file_system/physical_file_system.dart';
 import 'package:analyzer_testing/utilities/extensions/resource_provider.dart';
-import 'package:path/path.dart' as path show Context;
+import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 import 'package:test_reflective_loader/test_reflective_loader.dart';
 
@@ -17,25 +20,55 @@ void main() {
   });
 }
 
+/// Test the [SessionLoggerFileSink], an implementation of [SessionLoggerSink]
+/// that writes to the physical disk via `dart:io`. It doesn't use the
+/// analyzer file abstraction, as it uses `openWrite()` and `IOSink` which are
+/// not implemented in the abstraction.
 @reflectiveTest
 class SessionLoggerFileSinkTest {
   late LogNormalizer normalizer;
-  late MemoryResourceProvider provider;
-  late path.Context pathContext;
+  late io.Directory tempDirectory;
+  late String logPath;
+  late io.File logFile;
+  PhysicalResourceProvider provider = PhysicalResourceProvider.INSTANCE;
+  late path.Context pathContext = provider.pathContext;
+
+  late String Function(String) convertPath = ResourceProviderExtension(
+    provider,
+  ).convertPath;
 
   void setUp() {
-    provider = MemoryResourceProvider();
-    pathContext = provider.pathContext;
     normalizer = LogNormalizer();
+    tempDirectory = io.Directory.systemTemp.createTempSync(
+      'dartServer_sessionLog_fileSinkTest',
+    );
+    logPath = path.join(tempDirectory.path, 'foo.txt');
+    logFile = io.File(logPath);
+  }
+
+  void tearDown() {
+    tempDirectory.deleteSync(recursive: true);
+  }
+
+  Future<void> test_multipleWrites() async {
+    var fileSink = SessionLoggerFileSink(logPath, normalizer: normalizer);
+
+    // Write multiple entries
+    fileSink.writeLogEntry({'id': 1});
+    fileSink.writeLogEntry({'id': 2});
+    fileSink.writeLogEntry({'id': 3});
+
+    await fileSink.close();
+
+    // Ensure they are all recorded.
+    var content = io.File(logPath).readAsStringSync();
+    expect(content, '{"id":1}\n{"id":2}\n{"id":3}\n');
   }
 
   Future<void> test_normalized() async {
-    var convertPath = ResourceProviderExtension(provider).convertPath;
-    var logPath = convertPath('/foo.txt');
     var pathToNormalize = convertPath('/path/to/normalize');
 
-    var logFile = provider.getFile(logPath);
-    var fileSink = SessionLoggerFileSink(logFile, normalizer: normalizer);
+    var fileSink = SessionLoggerFileSink(logPath, normalizer: normalizer);
     normalizer.addReplacementsForPath(pathToNormalize, 'normalized');
     fileSink.writeLogEntry({
       'kind': 'message',
@@ -49,7 +82,9 @@ class SessionLoggerFileSinkTest {
       },
     });
 
-    var content = logFile.readAsStringSync();
+    await fileSink.close();
+
+    var content = io.File(logPath).readAsStringSync();
     expect(
       content,
       '{"kind":"message",'