9678dcf · parseable-macos

git.iko.soy/parseable-macos

Fix 4 bugs: timer deinit thread safety, ISO8601 formatter concurrency, QueryViewModel cache race, SQL identifier ASCII alignment

9678dcfiko3 months ago

ParseableViewer/Models/SQLTokenizer.swift+12−2

compare with master

unifiedsplit

@@ -131,8 +131,8 @@ struct SQLTokenizer: Sendable {

131 tokens.append(Token(kind: .number(String(sql[start..<i])), range: start..<i))

132

133 // Identifier or keyword

134− case _ where c.isLetter || c == "_":

134+ case _ where isASCIILetter(c) || c == "_":

135− advance(&i, in: sql, while: { $0.isLetter || $0.isNumber || $0 == "_" })

135+ advance(&i, in: sql, while: { isASCIILetter($0) || isASCIIDigit($0) || $0 == "_" })

136 let word = String(sql[start..<i])

137 if keywords.contains(word.uppercased()) {

138 tokens.append(Token(kind: .keyword(word.uppercased()), range: start..<i))

@@ -264,6 +264,16 @@ struct SQLTokenizer: Sendable {

264 c.isASCII && c.isNumber

265 }

266

267+ /// Bare (unquoted) SQL identifiers use only ASCII letters. `Character.isLetter`

268+ /// matches the full Unicode letter range (`café`, `naïve`, etc.), which

269+ /// DataFusion's lexer would require to be double-quoted — accepting them bare

270+ /// here would tokenize them as identifiers and diverge token offsets from the

271+ /// server, throwing off `errorHighlightRange` mapping (same rationale as

272+ /// `isASCIIDigit`).

273+ private static func isASCIILetter(_ c: Character) -> Bool {

274+ c.isASCII && c.isLetter

275+ }

276+

267 private static func consumeNumber(_ i: inout String.Index, in sql: String) {

277 private static func consumeNumber(_ i: inout String.Index, in sql: String) {

268 // Digits with at most one decimal point, so malformed input like

278 // Digits with at most one decimal point, so malformed input like

269 // `1.2.3` doesn't get swallowed into a single number token.

279 // `1.2.3` doesn't get swallowed into a single number token.

ParseableViewer/Services/ParseableClient.swift+9−4

@@ -101,11 +101,15 @@ final class ParseableClient: Sendable {

101 // so a single shared instance would be a data race. Constructing one is cheap.

102 private static var jsonDecoder: JSONDecoder { JSONDecoder() }

103

104+ // A fresh formatter per access, for the same reason as `jsonDecoder` above:

105+ // `query` can run concurrently and `ISO8601DateFormatter.string(from:)` isn't

106+ // documented as thread-safe, so a single shared instance would contradict the

107+ // file's per-call concurrency discipline. Constructing one is cheap.

104− private static nonisolated(unsafe) let isoFormatter: ISO8601DateFormatter = {

108+ private static var isoFormatter: ISO8601DateFormatter {

105 let f = ISO8601DateFormatter()

109 let f = ISO8601DateFormatter()

106 f.formatOptions = [.withInternetDateTime, .withFractionalSeconds]

110 f.formatOptions = [.withInternetDateTime, .withFractionalSeconds]

107 return f

111 return f

108− }()

112+ }

109

113

110 init(url: URL, username: String, password: String) {

114 init(url: URL, username: String, password: String) {

111 self.baseURL = url

115 self.baseURL = url

@@ -288,10 +292,11 @@ final class ParseableClient: Sendable {

288 // MARK: - Query

292 // MARK: - Query

289

293

290 func query(sql: String, startTime: Date, endTime: Date) async throws -> [LogRecord] {

294 func query(sql: String, startTime: Date, endTime: Date) async throws -> [LogRecord] {

295+ let formatter = Self.isoFormatter

291 let body: [String: Any] = [

296 let body: [String: Any] = [

292 "query": sql,

297 "query": sql,

293− "startTime": Self.isoFormatter.string(from: startTime),

298+ "startTime": formatter.string(from: startTime),

294− "endTime": Self.isoFormatter.string(from: endTime)

299+ "endTime": formatter.string(from: endTime)

295 ]

300 ]

296

301

297 let bodyData = try JSONSerialization.data(withJSONObject: body)

302 let bodyData = try JSONSerialization.data(withJSONObject: body)

ParseableViewer/ViewModels/LiveTailViewModel.swift+9−1

@@ -57,7 +57,15 @@ final class LiveTailViewModel {

57 private var pollGeneration = 0

59 deinit {

60− timer?.invalidate()

60+ // Timer.invalidate() must run on the run loop the timer was scheduled on

61+ // (RunLoop.main, see start()). deinit is nonisolated and can run off the

62+ // main thread, so hop to the main queue rather than invalidating inline.

63+ // The scheduled timer only weakly references self, so deferring teardown

64+ // past deallocation is safe — at worst it fires once more as a no-op

65+ // before being invalidated. stop() remains the normal teardown path.

66+ if let timer {

67+ DispatchQueue.main.async { timer.invalidate() }

68+ }

61 }

69 }

63 // Cached formatters — nonisolated(unsafe) so static methods can use them from Task.detached

71 // Cached formatters — nonisolated(unsafe) so static methods can use them from Task.detached

ParseableViewer/ViewModels/QueryViewModel.swift+11

@@ -17,6 +17,8 @@ final class QueryViewModel {

17 // Build search texts asynchronously

18 searchTextTask?.cancel()

19 cachedSearchTexts = []

20+ searchTextGeneration &+= 1

21+ let generation = searchTextGeneration

20 // Apply the current filter immediately using the inline fallback path

22 // Apply the current filter immediately using the inline fallback path

21 // (the cache was just invalidated). Calling updateFilteredResults()

23 // (the cache was just invalidated). Calling updateFilteredResults()

22 // rather than `filteredResults = results` avoids briefly showing

24 // rather than `filteredResults = results` avoids briefly showing

@@ -36,6 +38,12 @@ final class QueryViewModel {

36 }

38 }

37 }.value

39 }.value

38 guard let self, !Task.isCancelled else { return }

40 guard let self, !Task.isCancelled else { return }

41+ // Reject a stale write: if `results` was reassigned while this task

42+ // was awaiting, a newer didSet bumped the generation. Task.isCancelled

43+ // alone isn't sufficient because two re-queries producing result sets

44+ // of identical count would otherwise let this stale, same-length

45+ // cachedSearchTexts slip past updateFilteredResults's count guard.

46+ guard generation == self.searchTextGeneration else { return }

39 self.cachedSearchTexts = texts

47 self.cachedSearchTexts = texts

40 if !self.filterText.isEmpty {

48 if !self.filterText.isEmpty {

41 self.updateFilteredResults()

49 self.updateFilteredResults()

@@ -45,6 +53,9 @@ final class QueryViewModel {

45 }

53 }

46 private var cachedSearchTexts: [String] = []

54 private var cachedSearchTexts: [String] = []

47 private var searchTextTask: Task<Void, Never>?

55 private var searchTextTask: Task<Void, Never>?

56+ /// Bumped on every `results` reassignment so the async search-text builder can

57+ /// detect it was superseded and drop a stale write (see results.didSet).

58+ private var searchTextGeneration = 0

48 var columns: [String] = []

59 var columns: [String] = []

49 var columnOrder: [String] = []

60 var columnOrder: [String] = []

50 var hiddenColumns: Set<String> = []

61 var hiddenColumns: Set<String> = []