fix memory leak in earnings tab
This commit is contained in:
parent
609f3aacd2
commit
a875fc3b32
5 changed files with 101 additions and 16 deletions
29
src/cache/store.zig
vendored
29
src/cache/store.zig
vendored
|
|
@ -1955,6 +1955,35 @@ test "dividend serialize/deserialize round-trip" {
|
|||
try std.testing.expectEqual(DividendType.special, parsed[1].type);
|
||||
}
|
||||
|
||||
test "earnings serialize/deserialize round-trip frees duped symbols" {
|
||||
// Regression test for the earnings-tab memory leak: the cache-read
|
||||
// path dupes each event's `symbol` string into the caller's
|
||||
// allocator (string-bearing type, so `parse_allocator = .custom`).
|
||||
// Freeing only the outer slice leaks every symbol. `freeSlice` must
|
||||
// release them; testing.allocator fails the test on any leak.
|
||||
const io = std.testing.io;
|
||||
const allocator = std.testing.allocator;
|
||||
const events = [_]EarningsEvent{
|
||||
.{ .symbol = "AAPL", .date = Date.fromYmd(2025, 1, 30), .estimate = 2.67, .actual = 2.85 },
|
||||
.{ .symbol = "AAPL", .date = Date.fromYmd(2024, 10, 31), .estimate = 1.55, .actual = 1.64 },
|
||||
};
|
||||
|
||||
const data = try Store.serializeWithMeta(EarningsEvent, io, allocator, &events, .{});
|
||||
defer allocator.free(data);
|
||||
|
||||
const result = Store.readSlice(EarningsEvent, io, allocator, data, null, .any) orelse return error.TestUnexpectedResult;
|
||||
// The fix: route the free through freeSlice so the duped symbols go
|
||||
// too. Before the fix this was `allocator.free(result.data)` and the
|
||||
// symbols leaked.
|
||||
defer EarningsEvent.freeSlice(allocator, result.data);
|
||||
|
||||
try std.testing.expectEqual(@as(usize, 2), result.data.len);
|
||||
// Symbols survived the iterator teardown (proves they were duped).
|
||||
try std.testing.expectEqualStrings("AAPL", result.data[0].symbol);
|
||||
try std.testing.expectEqualStrings("AAPL", result.data[1].symbol);
|
||||
try std.testing.expect(result.data[0].date.eql(Date.fromYmd(2025, 1, 30)));
|
||||
}
|
||||
|
||||
test "split serialize/deserialize round-trip" {
|
||||
const io = std.testing.io;
|
||||
const allocator = std.testing.allocator;
|
||||
|
|
|
|||
|
|
@ -48,6 +48,27 @@ pub const EarningsEvent = struct {
|
|||
if (est == 0) return null;
|
||||
return ((act - est) / @abs(est)) * 100.0;
|
||||
}
|
||||
|
||||
/// Free any owned string fields.
|
||||
///
|
||||
/// `symbol` is heap-allocated by both producer paths: the cache-read
|
||||
/// path dupes it into the caller's allocator (SRF `parse_allocator`,
|
||||
/// see `cache/store.zig`), and the FMP provider path dupes it in
|
||||
/// `parseEarningsResponse`. The `len > 0` guard skips the empty-string
|
||||
/// default used for in-memory/test construction, which points at a
|
||||
/// string literal rather than the heap.
|
||||
pub fn deinit(self: EarningsEvent, allocator: std.mem.Allocator) void {
|
||||
if (self.symbol.len > 0) allocator.free(self.symbol);
|
||||
}
|
||||
|
||||
/// Free a slice of events, calling `deinit` on each element first.
|
||||
/// Mirror of `Dividend.freeSlice` — this is what makes
|
||||
/// `FetchResult(EarningsEvent).deinit()` release the per-event
|
||||
/// `symbol` strings instead of just the outer slice.
|
||||
pub fn freeSlice(allocator: std.mem.Allocator, events: []const EarningsEvent) void {
|
||||
for (events) |e| e.deinit(allocator);
|
||||
allocator.free(events);
|
||||
}
|
||||
};
|
||||
|
||||
const std = @import("std");
|
||||
|
|
@ -88,3 +109,26 @@ test "surprisePct" {
|
|||
const miss = EarningsEvent{ .symbol = "AAPL", .date = Date{ .days = 19000 }, .actual = 1.35, .estimate = 1.50 };
|
||||
try std.testing.expect(miss.surprisePct().? < 0);
|
||||
}
|
||||
|
||||
test "freeSlice releases each event's owned symbol" {
|
||||
// Mirrors the cache-read / provider path: each event's `symbol` is
|
||||
// heap-allocated and owned by the slice's allocator. freeSlice must
|
||||
// release every symbol plus the outer slice. testing.allocator fails
|
||||
// the test on any leak or double-free.
|
||||
const allocator = std.testing.allocator;
|
||||
const events = try allocator.alloc(EarningsEvent, 2);
|
||||
events[0] = .{ .symbol = try allocator.dupe(u8, "AAPL"), .date = Date{ .days = 19000 } };
|
||||
events[1] = .{ .symbol = try allocator.dupe(u8, "MSFT"), .date = Date{ .days = 19001 } };
|
||||
EarningsEvent.freeSlice(allocator, events);
|
||||
}
|
||||
|
||||
test "deinit tolerates the empty-string default (literal, not heap)" {
|
||||
// An event built with the default symbol points at a string literal,
|
||||
// not the heap. deinit must not attempt to free it.
|
||||
const ev = EarningsEvent{ .date = Date{ .days = 19000 } };
|
||||
ev.deinit(std.testing.allocator);
|
||||
}
|
||||
|
||||
test "freeSlice on an empty slice is a no-op" {
|
||||
EarningsEvent.freeSlice(std.testing.allocator, &.{});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -104,7 +104,10 @@ fn parseEarningsResponse(
|
|||
};
|
||||
|
||||
var events: std.ArrayList(EarningsEvent) = .empty;
|
||||
errdefer events.deinit(allocator);
|
||||
errdefer {
|
||||
for (events.items) |e| e.deinit(allocator);
|
||||
events.deinit(allocator);
|
||||
}
|
||||
|
||||
for (items) |item| {
|
||||
const obj = switch (item) {
|
||||
|
|
@ -139,8 +142,14 @@ fn parseEarningsResponse(
|
|||
const period_anchor = date.subtractMonths(1);
|
||||
const quarter: u8 = @intCast(((period_anchor.month() - 1) / 3) + 1);
|
||||
|
||||
try events.append(allocator, .{
|
||||
.symbol = symbol,
|
||||
// Each event owns its own copy of `symbol`. The cache-read path
|
||||
// dupes string fields into the caller's allocator, so the provider
|
||||
// path must too — otherwise the returned slice has mixed ownership
|
||||
// and no single `freeSlice` can release it correctly. See
|
||||
// `EarningsEvent.deinit`.
|
||||
const owned_symbol = try allocator.dupe(u8, symbol);
|
||||
events.append(allocator, .{
|
||||
.symbol = owned_symbol,
|
||||
.date = date,
|
||||
.estimate = estimate,
|
||||
.actual = actual,
|
||||
|
|
@ -152,7 +161,10 @@ fn parseEarningsResponse(
|
|||
.revenue_estimate = optFloat(obj.get("revenueEstimated")),
|
||||
// FMP's /stable/earnings endpoint doesn't expose BMO/AMC timing.
|
||||
.report_time = .unknown,
|
||||
});
|
||||
}) catch |err| {
|
||||
allocator.free(owned_symbol);
|
||||
return err;
|
||||
};
|
||||
}
|
||||
|
||||
// Newest-first, matching the UI's expectation.
|
||||
|
|
@ -181,7 +193,7 @@ test "parseEarningsResponse: typical response (AMZN-style)" {
|
|||
;
|
||||
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "AMZN");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 3), events.len);
|
||||
|
||||
|
|
@ -216,7 +228,7 @@ test "parseEarningsResponse: AAPL fiscal-year company quarter mapping" {
|
|||
;
|
||||
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "AAPL");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 1), events.len);
|
||||
try testing.expectEqual(@as(?u8, 4), events[0].quarter);
|
||||
|
|
@ -234,7 +246,7 @@ test "parseEarningsResponse: skips empty stub records" {
|
|||
;
|
||||
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "SPY");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 0), events.len);
|
||||
}
|
||||
|
|
@ -247,7 +259,7 @@ test "parseEarningsResponse: keeps records with only estimate (upcoming)" {
|
|||
\\]
|
||||
;
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "X");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 1), events.len);
|
||||
try testing.expect(events[0].actual == null);
|
||||
|
|
@ -264,7 +276,7 @@ test "parseEarningsResponse: keeps records with only actual (no estimate)" {
|
|||
\\]
|
||||
;
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "AAPL");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 1), events.len);
|
||||
try testing.expectApproxEqAbs(@as(f64, 0.00161), events[0].actual.?, 1e-6);
|
||||
|
|
@ -279,13 +291,13 @@ test "parseEarningsResponse: error envelope returns empty" {
|
|||
\\{"Error Message": "API limit reached"}
|
||||
;
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "X");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
try testing.expectEqual(@as(usize, 0), events.len);
|
||||
}
|
||||
|
||||
test "parseEarningsResponse: empty array" {
|
||||
const events = try parseEarningsResponse(testing.allocator, "[]", "X");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
try testing.expectEqual(@as(usize, 0), events.len);
|
||||
}
|
||||
|
||||
|
|
@ -302,7 +314,7 @@ test "parseEarningsResponse: surprise_percent handles zero estimate" {
|
|||
\\]
|
||||
;
|
||||
const events = try parseEarningsResponse(testing.allocator, body, "X");
|
||||
defer testing.allocator.free(events);
|
||||
defer EarningsEvent.freeSlice(testing.allocator, events);
|
||||
|
||||
try testing.expectEqual(@as(usize, 1), events.len);
|
||||
try testing.expectApproxEqAbs(@as(f64, 0.05), events[0].surprise.?, 0.001);
|
||||
|
|
|
|||
|
|
@ -939,7 +939,7 @@ pub const DataService = struct {
|
|||
return .{ .data = cached.data, .source = .cached, .timestamp = cached.timestamp, .allocator = self.allocator };
|
||||
}
|
||||
// Stale: free cached events and re-fetch below
|
||||
self.allocator.free(cached.data);
|
||||
EarningsEvent.freeSlice(self.allocator, cached.data);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -65,7 +65,7 @@ pub const tab = struct {
|
|||
|
||||
/// One-time teardown. Free any allocated payloads.
|
||||
pub fn deinit(state: *State, app: *App) void {
|
||||
if (state.data) |e| app.allocator.free(e);
|
||||
if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e);
|
||||
state.* = .{};
|
||||
}
|
||||
|
||||
|
|
@ -92,7 +92,7 @@ pub const tab = struct {
|
|||
}
|
||||
// Clear every flag so loadData has the same starting
|
||||
// conditions as a fresh activation.
|
||||
if (state.data) |e| app.allocator.free(e);
|
||||
if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e);
|
||||
state.* = .{};
|
||||
loadData(state, app);
|
||||
}
|
||||
|
|
@ -111,7 +111,7 @@ pub const tab = struct {
|
|||
/// next `activate` re-fetches for the new symbol. Distinct
|
||||
/// from `reload` (no fetch is triggered here).
|
||||
pub fn onSymbolChange(state: *State, app: *App) void {
|
||||
if (state.data) |e| app.allocator.free(e);
|
||||
if (state.data) |e| zfin.EarningsEvent.freeSlice(app.allocator, e);
|
||||
state.* = .{};
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue