Release aggregate instances when sqlite finishes with them

[djmb]

The check at the top of the destroy function returns when the aggregate context still holds an instance, which is exactly when there is something to clean up. So instances were never unlinked, and every aggregate call left one behind for as long as the connection stayed open.

[djmb]

@flavorjones - here's a script to reproduce the leak. It creates a custom aggregator and then queries against it continually:

ruby sqlite3-aggregate-leak.rb                                    # released gem
SQLITE3_PATH=/path/to/sqlite3-ruby ruby sqlite3-aggregate-leak.rb # this branch

Before — sqlite3 2.9.5:

                         handlers      +/- slots         +/- MB
baseline                        1             +0           +0.0
after 200 queries          100001        +400036          +85.0
after 400 queries          200001        +400002         +124.0
after 600 queries          300001        +400006         +118.8
after 800 queries          400001        +400000         +126.1
after 1000 queries         500001        +400000         +124.7

Over 1000 queries: 500000 handlers leaked, 2000045 slots, +579 MB.
That is one handler per group per query (1000 x 500).

After — this branch:

                         handlers      +/- slots         +/- MB
baseline                        1             +0           +0.0
after 200 queries               1            +36          -19.4
after 400 queries               1             +2           +0.0
after 600 queries               1             +6           +0.0
after 800 queries               1             +0           +0.0
after 1000 queries              1             +0           +0.0

No leak: handlers stayed at 1.

sqlite3-aggregate-leak.rb

#!/usr/bin/env ruby
#
# Demonstrates that sqlite3-ruby never releases custom aggregate instances.
#
# sqlite creates one aggregate context per GROUP BY group, and the gem creates a
# handler instance per context. rb_sqlite3_aggregate_instance_destroy is supposed
# to unlink each one when sqlite calls xFinal, but its guard is inverted:
#
#   if (!inst_ptr || (inst = *inst_ptr)) {   /* returns when there IS an instance */
#       return;
#   }
#
# so the cleanup only runs when there is nothing to clean up. Every instance
# stays in the wrapper's -instances array for the life of the connection, along
# with whatever state the handler accumulated.
#
#   ruby sqlite3-aggregate-leak.rb                                  # released gem
#   SQLITE3_VERSION=2.9.4 ruby sqlite3-aggregate-leak.rb            # another release
#   SQLITE3_PATH=~/src/sqlite3-ruby ruby sqlite3-aggregate-leak.rb  # a local checkout
#
# Expected: 1 live handler (the template) no matter how many queries run.

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  if (path = ENV["SQLITE3_PATH"])
    gem "sqlite3", path: path
  else
    gem "sqlite3", ENV.fetch("SQLITE3_VERSION", "2.9.5")
  end
end

require "sqlite3"

ROWS = 50_000
GROUPS = 500
QUERIES = 1_000

# A realistic accumulating aggregate: it has to keep every value it sees, so a
# leaked instance retains an array rather than just an object header.
class Median
  def step(value)
    @values ||= []
    @values << value.to_f
  end

  def finalize
    sorted = @values.sort
    sorted[sorted.length / 2]
  end
end

def live_handlers
  GC.start(full_mark: true, immediate_sweep: true)
  ObjectSpace.each_object(Median).count
end

def live_slots
  GC.start(full_mark: true, immediate_sweep: true)
  GC.stat(:heap_live_slots)
end

def rss_mb
  File.read("/proc/self/status")[/VmRSS:\s+(\d+)/, 1].to_i / 1024.0
rescue StandardError
  nil
end

PREVIOUS = {}
START = {}

# Each row is the change since the row above it, so a steady figure means the
# leak is still going rather than having been a one-off cost.
def report(label)
  slots = live_slots
  rss = rss_mb
  slots_delta = PREVIOUS[:slots] ? slots - PREVIOUS[:slots] : 0
  rss_delta = rss && PREVIOUS[:rss] ? rss - PREVIOUS[:rss] : 0.0
  PREVIOUS[:slots] = slots
  PREVIOUS[:rss] = rss
  START[:slots] ||= slots
  START[:rss] ||= rss

  printf "%-22s %10d %14s %14s\n",
    label,
    live_handlers,
    format("%+d", slots_delta),
    rss ? format("%+.1f", rss_delta) : "n/a"
end

db = SQLite3::Database.new(":memory:")
db.execute "create table t (value real, grp integer)"
db.transaction do
  ROWS.times { |i| db.execute "insert into t values (?, ?)", [i * 1.5, i % GROUPS] }
end
db.define_aggregator "median", Median.new

puts "sqlite3 gem #{SQLite3::VERSION}, sqlite #{SQLite3::SQLITE_VERSION}, ruby #{RUBY_VERSION}"
puts "#{ROWS} rows over #{GROUPS} groups, #{QUERIES} queries"
puts

# Heap slots is the exact signal — four per leaked handler, linear from the first
# sample. Resident memory is the one that shows why it matters, though it lags
# for the first few dozen queries while bundler hands back what it allocated
# while resolving.
INTERVAL = QUERIES / 5

puts "slots and MB are the change since the row above, #{INTERVAL} queries earlier"
puts
printf "%-22s %10s %14s %14s\n", "", "handlers", "+/- slots", "+/- MB"
report "baseline"

QUERIES.times do |i|
  db.execute "select grp, median(value) from t group by grp"
  report "after #{i + 1} queries" if ((i + 1) % INTERVAL).zero?
end

puts
leaked = live_handlers - 1
if leaked.zero?
  puts "No leak: handlers stayed at 1."
else
  total_slots = live_slots - START[:slots]
  total_rss = rss_mb ? format(", %+.0f MB", rss_mb - START[:rss]) : ""
  puts "Over #{QUERIES} queries: #{leaked} handlers leaked, #{total_slots} slots#{total_rss}."
  puts "That is one handler per group per query (#{QUERIES} x #{GROUPS})."
  puts "Nothing is released until the Database object is garbage collected, so a"
  puts "pooled connection accumulates these for the life of the process."
end