9Reading, Debugging and Software Design

Answer to Question 9.1.

It prints more bids than asks, then 18446744073709551615. Nothing is undefined. The fix is to compare the sizes directly: bids.size() > asks.size().

size() returns std::size_t, an unsigned type of 64 bits here. The standard defines unsigned arithmetic modulo $2^{64}$, so $2 - 3$ gives $2^{64} - 1$, which is the second line of output. An unsigned value is never negative, so > 0 is true for every pair of sizes that differ, whichever is larger. Under -Wall -Wextra -Werror the listing compiles without a word, because > 0 on an unsigned value is a sensible test in general.

If you do need the signed difference, C++20 has std::ssize. Subtracting two of those and testing > 0 also gives the right branch.

Answer to Question 9.2.

It prints [30, 30, 30]. To get [10, 20, 30], bind the value when the function is created: lambda x, k=k: k * x. A list comprehension does not help.

A closure in Python captures the variable, not its value. All three lambdas refer to the same k in the scope of make_scalers, and they look it up when called. By then the loop has finished and k is 3. Default arguments are evaluated when the function is defined, which is why k=k takes a snapshot. functools.partial does the same and leaves no extra parameter that a caller could pass by accident.

Answer to Question 9.3.

On this compiler it prints 10 0 20, one zero survives, and the program is undefined. With a zero in last place it runs off the end of the vector. Use std::erase(qty, 0L).

erase invalidates every iterator at or after the erased element, so it may no longer be used, and ++it is already undefined. What happened in the run: the elements slid left, so it pointed at the second zero, and ++it stepped over it. That is why one zero survives.

Now make the input {10, 0}. The erase leaves it equal to end(), the ++it moves past it, and the test it != qty.end() never becomes false. AddressSanitizer reports a heap-buffer-overflow on that input. The libstdc++ debug mode, -D_GLIBCXX_DEBUG, stops even the first input, with a message about a singular iterator.

C++20’s std::erase does the job in one pass. Before C++20 you would write the erase-remove idiom. The manual loop can be repaired with it = qty.erase(it) and no increment on that branch, but each erase shifts the tail, so it costs $O(n^2)$ where std::erase costs $O(n)$.

Answer to Question 9.4.

It prints 1, 1, 2. The wrong answer: a symbol with no limit is allowed. The side effect: asking about BBB inserts it, so the map grows from one entry to two.

std::map::operator[] is defined as try_emplace(x).first->second: if the key is absent, it inserts a value-initialised entry, here 0, and returns a reference to it. So limits["BBB"] creates a limit of 0, and $0 \le 0$ passes. A risk check that says yes to an instrument it has never heard of should fail closed.

To make the compiler catch it, make the map const, or take it by const&. operator[] has no const overload, so the code stops compiling: the test does this. Then write the lookup with find:

const auto it = limits.find(symbol);

return it != limits.end() && qty <= it->second;

That prints 1, 0, 1. at is the other choice, and it throws on a missing key.

Answer to Question 9.5.

It prints start base, then run momentum. During Strategy’s constructor the object is a Strategy, so the call goes to Strategy::name.

The standard says that a virtual function called from a constructor resolves to the final overrider in the constructor’s own class, not in a more-derived one. The reason is sound: Momentum’s members do not exist yet, and an override might use them. Once construction ends, m.name() sees Momentum as expected.

If name is pure and the constructor calls it directly, the compiler can complain. Through a helper it cannot see the problem. That call is undefined, and at -O0 the test run aborts with pure virtual method called.

To get what the author wanted, separate construction from starting. Either have a factory that builds the object and then calls start() on it, or pass the name up through the constructor: Strategy(const char* name).

Answer to Question 9.6.

It prints 0.0 101.0 0.0. This is object slicing. Store pointers in the vector, usually unique_ptr, and delete the base class’s copy constructor to make the mistake fail to compile.

A vector of Instrument holds objects of exactly that type, each with room for an Instrument only. push_back(Bond(99.5)) copies only the Instrument part, so the price is gone and the virtual table pointer is Instrument’s: book[0].value() is 0.0. copy is the same thing written out: Instrument copy = b. Only ref is still a Bond, so it gives 101.0.

With the vector of unique_ptr, the first number becomes 99.5. Add Instrument(const Instrument&) = delete;, together with a defaulted default constructor, and both slicing lines stop compiling. That is the usual rule for a class meant to be used through a base pointer: no public copying. If you do need copies, add a virtual clone.

Answer to Question 9.7.

Under cargo run it panics, and the message is attempt to subtract with overflow. With --release it prints $2^{64} - 1$ in full. Neither is unsafe in Rust’s sense. Use abs_diff for the size of the gap, or a signed result if the sign matters.

len() returns usize, and $2 - 3$ does not fit. The Rust Reference calls arithmetic overflow a programmer’s error, but it is not undefined behaviour. With debug assertions on, the compiler must insert a check that panics. In a release build the default is to wrap, as the test confirms. The program stays memory safe either way: the wrong number comes out, or the process stops, and no invariant of the language is broken. You can keep the checks in release builds with overflow-checks = true in the Cargo profile, and the test confirms that an optimised build then panics too.

The fix depends on the caller. bids.len().abs_diff(asks.len()) gives 1 here if only the size of the gap matters. checked_sub returns None if a negative result means “this should not happen”. a as i64 - b as i64 gives $-1$ if the caller wants the sign.

Answer to Question 9.8.

It prints False 0, then 2.67 0 2 2. Tell your colleague to key the levels by an integer number of ticks.

$0.1 + 0.2$ is 0.30000000000000004 in binary floating point, and the literal 0.3 is a different double. Dict lookup uses exact equality, so the entry exists under one key and the lookup asks for another, and the fill appears to vanish. Two prices that print the same can differ in the last bit if they came by different arithmetic.

The second line shows two more traps in round. 2.675 is stored slightly below that value, as 2.67499999…, so it rounds down, and the Python documentation says so itself. Halves go to the even neighbour: round(0.5) is 0, round(1.5) and round(2.5) are both 2. Rounding to the tick’s decimals does give one canonical double, so round(0.1 + 0.2, 2) == 0.3, but it works only while the error stays far below half a tick, and each writer must remember to do it. Integer ticks need no such care.

They solve it completely: at a tick of 0.001, the keys 100 and 200 add to exactly 300. Decimal("0.1") + Decimal("0.2") also equals Decimal("0.3"), which suits reports and money, at a cost in speed.

Answer to Question 9.9.

It prints true false true and true false true. The language promises every result here except c == d: only that false belongs to this JVM.

== on references asks whether they are the same object. The Java Language Specification says that boxing an integer between $-128$ and 127 twice gives the same object, so a == b is always true. Beyond that range nothing is promised either way. The specification adds that less memory-limited implementations might cache more, up to $\pm$32K for int. OpenJDK 11 with default settings gives two objects for 128, so c == d is false.

For strings, the specification says literals are interned, so s == t is true. new String("AAA") is a new object by definition, so s == u is false, while equals compares contents.

The rule is short: use equals on objects, and == only on primitives. This bug is nasty because it passes every test with small quantities.

Answer to Question 9.10.

It prints 100, then ~Order, and ~LimitOrder never runs. The program is undefined: deleting a LimitOrder through an Order* needs a virtual destructor. AddressSanitizer stops it. With std::make_shared<LimitOrder> it is defined, and both destructors run.

The standard says that in a single-object delete, if the static type differs from the dynamic type, the static type must have a virtual destructor, or the behaviour is undefined. qty is virtual, so the class is polymorphic, but the destructor is not. In this run, tag is never destroyed and its heap buffer is lost.

ASan reports new-delete-type-mismatch: the allocated type has 40 bytes and the deallocated one 8. With that check turned off, it reports the leak of the string instead. The test shows both.

Why was there no warning? g++ has -Wdelete-non-virtual-dtor, which -Wall turns on, but the delete expression here is inside unique_ptr, in a system header, where warnings are suppressed. Write delete p on a raw pointer and the compile fails under the same flags, as the test confirms. So do not trust the silence.

The fix is one word: virtual ~Order(). It then prints ~LimitOrder, then ~Order, and ASan is silent.

Answer to Question 9.11.

It leaks one Order every time validate throws. Here that is one object of 40 bytes, plus nothing else, because the short tag fits in the string’s own buffer. LeakSanitizer finds it. Use std::make_unique. Do not accept the catch.

The path is the -1 order. new succeeds, validate throws, the stack unwinds, and the raw pointer o is destroyed without touching what it points to. So delete never runs. The total is still right, 300, because the leak has nothing to do with the arithmetic, which is why it could run for months: a few bytes per rejected order.

ASan’s leak check reports Direct leak of 40 byte(s) in 1 object(s), allocated in handle. With auto o = std::make_unique<Order>(…) and no delete, the destructor runs during unwinding. The test confirms it: same output, nothing in the report.

The colleague’s version does work for one pointer. But it duplicates the cleanup on two paths. It gets worse with each new resource, because a second allocation needs nested handlers. And the next person to add an early return forgets it. This is why the rule is that every resource has an owning object whose destructor releases it. Here, in fact, you need no heap at all: const Order o{qty, "tag"}; is the simplest correct code.

Answer to Question 9.12.

The listing is undefined in C++20, and ASan reports stack-use-after-scope. The version with the public member is defined and gives 600. C++23 makes the first version defined too.

A range-for is defined as auto&& range = initializer; followed by a loop. Binding a reference to a temporary extends the life of that temporary, but only if the reference binds to it, or to a subobject of it. Here it binds to whatever levels() returns. That function returns a reference, and the compiler cannot see that it points into the temporary Snapshot. So the Snapshot dies at the end of the initialiser, before the loop starts, and the loop walks a destroyed vector.

latest().levels_ names a subobject of the temporary directly, so the extension applies to the whole Snapshot. The test runs it clean under ASan. So the same-looking line is safe through a data member and unsafe through a getter.

C++23 added a rule to [class.temporary]: a temporary created in a range-for’s initialiser lives as long as the range reference. g++ 11.4 in C++20 mode does not do this, as the ASan report shows.

For C++20, keep the object in a named variable. Write const Snapshot snap = latest(); first, then loop over snap.levels(). To make the compiler reject it, add reference qualifiers: levels() const& plus a deleted levels() const&&. The test shows the listing then fails to compile.

Answer to Question 9.13.

The line std::lock_guard<std::mutex>{m}; creates a temporary, which locks and unlocks at the semicolon. So total += n runs unprotected and there is a data race. Give the guard a name: std::lock_guard<std::mutex> lock{m};.

The object has no name, so its life ends with its full expression, before the next line. The compiler sees a legal expression with a constructor and destructor that have side effects, and gives no warning, even under -Wall -Wextra -Werror. ThreadSanitizer reports data race at once on total. Without it, the total may or may not come out right, depending on how the threads interleave. With the name added, TSan is silent and the program prints 200000.

With parentheses, std::lock_guard<std::mutex>(m); parses as a declaration of a variable called m of type lock_guard, with no initialiser. The extra parentheses around a declarator are allowed. Since lock_guard has no default constructor, it does not compile here, by luck: the test confirms it. For a type with a default constructor it would compile and hide the global m.

Answer to Question 9.14.

transfer(x, y) locks x then y, and transfer(y, x) locks y then x. If two such calls overlap, each can hold one mutex and wait for the other for ever. TSan does report it on this run, as lock-order-inversion. Use std::scoped_lock lock(from.m, to.m);.

This run cannot deadlock, because the first thread is joined before the second starts. That is the point of the question: you cannot catch this with a test that happens to pass. TSan does not need the deadlock to occur. It builds a graph of which mutex was held when another was acquired, and reports a cycle. Here it prints Cycle in lock order graph, on a run that printed the right answer.

std::scoped_lock with two mutexes uses std::lock, which the standard says does not result in deadlock. The usual technique is to lock one and try the others, backing off on failure. Another fix is a fixed global order, say by address or by account id. That also lets you see the rule in the code, and it costs no retries.

The self-transfer is the trap. With from and to the same account, scoped_lock tries to lock one mutex twice, and a std::mutex locked twice by one thread is undefined. In the test it hung until the timeout. So add a guard at the top: if (&from == &to) return;, or the equivalent rule for your domain.

Answer to Question 9.15.

The bug is the if: after wait returns, nothing checks that the queue still has an item. Use the predicate form, cv.wait(lock, [] { return !q.empty(); });. notify_one alone would not fix it.

Here is how it fails. Both consumers wait. push(7) calls notify_all, so both wake and compete for the mutex. The first takes 7. The second then gets the mutex, returns from wait, and goes straight to q.front() on an empty deque. That call is undefined. With no checks on, nothing stops the read, nor the pop_front after it, and this may pass unnoticed for a long while.

To make it loud, build with -D_GLIBCXX_ASSERTIONS, which turns on the library’s precondition checks. The test run aborts with SIGABRT at front. With the predicate the same build prints both values, and TSan is silent.

notify_one wakes one thread, which looks enough. It still leaves two ways to fail. First, wait is allowed to return with no notify at all: the standard’s wording includes “or spuriously”. Second, between the notify and the woken thread getting the mutex, a third thread calling pop can take the item, because it finds the queue non-empty and does not wait.

The rule is short: a condition variable never tells you the condition is true, only that it is worth checking again. So always wait in a loop or use the predicate form, which is that loop.

Answer to Question 9.16.

It prints copies 1, moves 1. The parameter is const Tracked, and std::move of a const object gives a const rvalue, which binds to the copy constructor. Drop the const and it prints copies 0, moves 2.

std::move moves nothing. It is a cast to an rvalue reference, and it keeps const. Overload resolution then has two candidates: Tracked(Tracked&&) needs a non-const argument, so it does not fit, and Tracked(const Tracked&) does. So the compiler quietly copies, with no warning. The one move is the outer one, from t into the parameter. The one copy is from the parameter into item_.

The const on a by-value parameter looks harmless and even virtuous, since it does not change the function’s signature. For a parameter you intend to move from, it is exactly wrong. The same goes for a const local variable that you later std::move.

In std::vector, growth has to give the strong exception guarantee: if something throws while relocating, the old buffer must still be intact. A move that throws would have destroyed the old elements. So the vector uses std::move_if_noexcept: it moves only if the move constructor is noexcept, or if the type cannot be copied. The test shows it: two emplace_back calls print copies 0, moves 1 with noexcept, and copies 1, moves 0 without it.

Answer to Question 9.17.

It is not correct: the second publish calls a lambda holding a dangling this, and it writes to freed memory. ASan reports heap-use-after-free at total_ += v. The fix gives the subscription an owner, so the destructor unsubscribes.

The feed stores the std::function, which stores the raw this. The feed knows nothing about the strategy’s lifetime. After s.reset(), the strategy is gone, but its lambda is still in subs_. Built normally, the write lands on freed memory and nothing shows, so you see 1, done. In production the same write lands in whatever was allocated there next, and the symptom appears somewhere else entirely.

The fix in Listing 9.41 makes subscribe return a handle, stored in the strategy, and the destructor gives it back. A list keeps iterators valid while other subscribers come and go. It runs clean under ASan.

There is a second bug to prevent. The default copy constructor would copy slot_ but leave the lambda pointing at the original object, and both destructors would erase the same slot. So the class must delete copying, as here. The test shows that copying then does not compile.

Another common design uses std::shared_ptr for the strategy and captures a std::weak_ptr in the lambda. The feed locks it on every call, and an expired one is skipped. That is simpler with several threads, and it costs an atomic operation per call.

class Feed {
public:
    using Slot = std::list<std::function<void(long)>>::iterator;
    Slot subscribe(std::function<void(long)> f) {
        subs_.push_back(std::move(f));
        return std::prev(subs_.end());
    }
    void unsubscribe(Slot s) { subs_.erase(s); }
    void publish(long v) { for (auto& f : subs_) f(v); }

private:
    std::list<std::function<void(long)>> subs_;
};

class Strategy {
public:
    explicit Strategy(Feed& feed)
        : feed_(feed), slot_(feed.subscribe([this](long v) { total_ += v; })) {}
    ~Strategy() { feed_.unsubscribe(slot_); }
    Strategy(const Strategy&) = delete;             // [this] is inside the feed: never copy
    Strategy& operator=(const Strategy&) = delete;
    long total() const { return total_; }

private:
    Feed& feed_;
    Feed::Slot slot_;
    long total_ = 0;
};

int main() {
    Feed feed;
    auto s = std::make_unique<Strategy>(feed);
    feed.publish(1);
    std::printf("%ld\n", s->total());
    s.reset();
    feed.publish(2);
    std::puts("done");
}

Answer to Question 9.18.

It prints False 1 True: the order is in the set, and in cannot find it. Hash on oid alone, the part that never changes, and have __eq__ agree. If you delete __hash__, book.add(o) raises a TypeError.

A set stores each item in a slot chosen from its hash at the time of insertion. Changing qty changes the hash that __hash__ now computes, so the lookup looks in a different slot. It finds nothing, and it never compares. The item is still there, as the last two values show: len is 1, and a scan by identity finds it. You now have an order that you can neither find nor remove.

The Python data model warns against this directly: a class that defines mutable objects and implements __eq__ should not implement __hash__, since a key’s hash value must be immutable. The language enforces half of this. A class that defines __eq__ without __hash__ has its hash set to None, so instances are unhashable. The test shows that error.

With hash(self.oid) and equality on oid, the line prints True 1 True. But note what that changes: two orders with the same id are now equal whatever their quantity. That is what you want for identity in a book. It is not what you want if you are comparing two snapshots. A cleaner design separates them: a dict from id to order keeps the identity in the key and leaves the order free to change.

Answer to Question 9.19.

It prints 150.0 and False: after del p and a full collection, the pricer is still alive. The cache keeps it alive, along with its million-element curve. Give each instance its own cache.

@lru_cache on a method wraps the function once, at class level. Its key contains all the arguments, and the first of them is self. So the class-level cache holds a strong reference to every instance that ever called it. The Python FAQ says the same of lru_cache on methods: “it creates a reference to the instance”. With maxsize=None it never evicts, so each new Pricer and its curve stay until the process ends. That is the memory growth. Linters flag this pattern too: ruff’s rule B019 reports this very line.

To keep the caching, create the cache in the constructor:

self.price = lru_cache(maxsize=None)(self._price)

The cache then belongs to the instance. There is now a reference cycle (instance to cache to bound method to instance), so the object is freed by the cycle collector rather than at once. The test prints True after gc.collect(). Bounding maxsize is sensible too: with the class-level form it limits the damage but does not cure it, because a few big instances can stay pinned.

Answer to Question 9.20.

It prints 1 day, 0:00:00, then 23:00:00. The second is true: on 8 March 2026 New York moved its clocks forward, so that day had 23 hours.

The Python documentation states the rule: if both datetimes are aware and have the same tzinfo attribute, the tzinfo attributes are ignored and no time zone adjustment is done. Here both use the same ny object, so the subtraction works on wall-clock fields, midnight to midnight, and gives one day. Converted to UTC, the two carry different offsets: the test shows $-5$ hours just before the change and $-4$ after. The subtraction then gives the elapsed time. The same trap runs the other way on 1 November 2026, a day of 25 hours.

Anything that divides by elapsed time goes wrong silently with the first form. Volume per hour, or a time-weighted average spread for that day, is off by about 4%, as 24 hours is used for a 23-hour day. A timeout or a session schedule computed by adding hours to a local time can be off by an hour. An annualisation or a time-to-expiry computed on local timestamps can pick up the same error.

The rule at trading firms: store and compute in UTC, or in integer nanoseconds since the epoch, and convert to local time only to display, or to apply exchange session rules, which are themselves defined in local time.

Answer to Question 9.21.

It prints 2 2 and nothing more. You never see drop a or drop b, because the two nodes own each other and neither count reaches zero. By Rust’s rules that is not a safety bug. Change Rc to Weak in the next field.

Each node starts with one strong reference from its local variable, and the other node adds one, hence 2 2. At the end of main, the two local variables go away and each count drops to 1. Nothing further can drop them, since each is kept alive by the other. So the memory is never freed and the destructors never run.

The standard library says so plainly: a cycle between Rc pointers will never be deallocated, and Weak is there to break cycles. The Rust Reference lists leaks of memory under behaviour that is not considered unsafe, along with deadlocks. So never rely on Drop to release a limit or close a position.

With RefCell<Weak<Node>> and Rc::downgrade, the fixed version prints 1 1, then a (the upgrade from b still succeeds), then drop b, drop a. A Weak does not keep the value alive, so upgrade returns an Option, and you must handle None.

Answer to Question 9.22.

On rustc 1.97 it hangs for ever. The guard from the first lock() lives until the end of the match, and the arm tries to lock the same mutex again. The 2024 edition does not change this. Add a let first: let item = queue.lock().unwrap().pop();, then match item {.

queue.lock().unwrap() creates a temporary MutexGuard, and pop() borrows from it only briefly. You might expect the guard to go once pop returns. But the Rust Reference notes that the scrutinee of a match is not a temporary scope, so temporaries in the scrutinee can be dropped after the match expression. The guard is still alive through the arms. The second lock() on the same thread then waits for a lock that it holds itself. The documentation of Mutex::lock says this call might panic. Here it hung, and the test sees the timeout, so do not rely on the panic.

The 2024 edition changes two scopes, and neither is this one. Temporaries in an if let scrutinee now drop before the else branch, but not before the body, and tail expression temporaries drop earlier. The test builds this match and the if let form under edition 2024, and both still hang, since the second lock is in the body.

With the let, the statement ends at its semicolon, the guard drops, and the program prints requeued 3, then done.

Answer to Question 9.23.

It prints true, then null. Key overrides equals but not hashCode, so two equal keys have different hash codes, and the map does not find the entry. Add hashCode, which returns symbol.hashCode().

The Object documentation states the contract: if two objects are equal according to equals, calling hashCode on each must produce the same integer result, so it is generally necessary to override hashCode whenever equals is overridden. The default hashCode is not based on the fields, so equal keys almost always have different hashes. HashMap.get compares the stored hash first, and calls equals only if they match, so even a key in the same bucket is not found. So the lookup returns null without ever calling your equals. With the override the program prints true and 1000.

The compiler says nothing, because both methods are optional. A null from get also looks like “no limit set”. That is the dangerous part. In a limit check, unboxing the null into a long gives a NullPointerException at best. At worst it reads as “no limit, so allow”.

Another way to break it: keep both methods, but use a mutable field in them, and change the field after inserting. The hash now differs from the stored one, and the same lookup returns null. This is why key classes should be immutable. Here symbol is final, and the class is final too, so a subclass cannot change what equality means. The test runs this mutable variant too.

Answer to Question 9.24.

It prints a number far below 400000, about 130000 in two runs here with four threads, and it changes from run to run. The map is thread-safe call by call, but get followed by put is two calls, so updates get lost between them. Replace both lines by position.merge(symbol, qty, Long::sum);.

Each operation of ConcurrentHashMap is safe, and the map’s internals are never corrupted. But thread A can read 5, thread B can read 5, both compute 6, and both write 6. One increment is gone. This is a check-then-act race, and no choice of collection saves you from it while the logic stays spread over two calls.

merge does the read, the computation and the write as one step on that key. The documentation states that the entire method invocation is performed atomically, and that update operations by other threads may be blocked meanwhile. So keep the function short, and do not touch the map from inside it. The test prints exactly 400000 with this line.

compute does the same with a more general function, and putIfAbsent covers the first-time case. Here, Long::sum is all that is needed.

For a counter hit by many threads and read rarely, use LongAdder. It keeps one or more variables that grow in number under contention, and adds them up on read. Its Javadoc calls it usually preferable to AtomicLong for such sums, at the expense of more space. map.computeIfAbsent(symbol, s -> new LongAdder()).add(qty) is the usual form.

Answer to Question 9.25.

It prints True: the strategy survives the collection. The event in feed holds a delegate that holds the strategy. Many short-lived strategies would therefore pile up, each still receiving ticks. Unsubscribe with -=, normally in Dispose.

feed.Tick += OnTick creates a delegate whose target is the Strategy instance, and adds it to the feed’s list. Garbage collection follows references, so a live feed keeps the delegate alive, which keeps the strategy alive. Microsoft’s programming guide says this in nearly those words: as long as the publishing object holds that reference, garbage collection will not delete your subscriber object, so you should unsubscribe before you dispose of the subscriber.

The listing is built with care to show this honestly. NoInlining on Make and the Release build mean that no local variable keeps the strategy alive by accident. GC.KeepAlive(feed) keeps the feed alive through the check. So the only path left is the event.

Short-lived subscribers on a long-lived publisher are the bad shape: an instrument’s view subscribing to a global feed, say, for the length of a screen. Memory grows, and so does CPU, because each dead strategy still runs OnTick on every tick, doing work for nothing. That second symptom is often the one you notice first.

With IDisposable and feed.Tick -= OnTick in Dispose, and using var s in Make, the program prints False.

Answer to Question 9.26.

Only the first can ever fail on a wrong number, and it cannot catch the bug. vwap ignores the quantities and takes a simple mean. Test with unequal quantities, and use pytest.raises and a bare assert.

Go through them. test_two_trades uses equal quantities, 50 and 50, and for those the simple and weighted means agree: 101 either way. test_empty has no assert and swallows the exception it hopes for, so it passes if the call raises that and passes if it returns. test_one_trade asserts a two-element tuple. A non-empty tuple is always true, so it can never fail, even though it compares against a wrong value, 99. Python itself warns at compile time: assertion is always true. The test captures that warning. pytest happily counts 3 passed.

The right function is the sum of $p \times q$ divided by the sum of $q$. Rewrite:

assert vwap([(100.0, 10), (102.0, 30)]) == 101.5

On the current function that gives 101, and it fails. Hand-compute your expected value, and do not paste it from a run.

with pytest.raises(ValueError): then vwap([]), which fails if nothing raises. Then assert vwap([(100.0, 7)]) == 100.0.

Add properties, which catch whole families of bugs: the result lies between the lowest and highest price, and multiplying every quantity by 3 changes nothing. The test runs both on 200 random lists.

To check the tests themselves, break the code on purpose and see that a test fails. This is the idea behind mutation testing, and here it would have exposed all three at once.

Answer to Question 9.27.

Give each quantity its own type: struct Price { long ticks; }; and struct Qty { long lots; };. send(qty, price) then does not compile. In the machine code, expect no difference at all from plain long.

Listing 9.42 is the result. It compiles at the same flags, prints the right thing, and swapping the arguments is now an error: the test confirms it. For the cost, it compares two functions:

long cost_raw(long, long) and long cost_typed(Price, Qty).

Under g++ 11.4 at -O2 on x86-64 Linux, both compile to the same instructions, movq, imulq, ret (plus an endbr64 on Ubuntu’s default flags). The test compares them line by line. The reason lies in the System V calling convention: a small struct of one integer goes in a register exactly as the integer does. A wrapper has no run-time existence, and all of it happens in the type checker.

Add only the operations that make sense. Price minus price is a price difference, and quantity times price is a notional. Price plus price has no meaning, so leave it out, and the compiler now stops that too. Conversions from long should be explicit, or the protection goes away.

There is one hole in this listing. These are aggregates, so send({500}, {101}) still compiles and still swaps. If that worries you, add an explicit constructor.

struct Price { long ticks; };
struct Qty { long lots; };

void send(Price price, Qty qty) {
    std::printf("%ld at %ld\n", qty.lots, price.ticks);
}

long cost_raw(long price, long qty) { return price * qty; }
long cost_typed(Price price, Qty qty) { return price.ticks * qty.lots; }

int main() {
    Price price{101};
    Qty qty{500};
    send(price, qty);
    std::printf("%ld %ld\n", cost_raw(101, 500), cost_typed(price, qty));
}

Answer to Question 9.28.

The plain bisect needs 10 runs, $2^{10} = 1024$, so 60 minutes. With the flaky test you need 24 runs per commit, and the worst case is 24 hours.

The git bisect command is a binary search: each test halves the range, so 1024 commits take 10 steps. That assumes one good commit and one later bad commit, with one change from good to bad between them.

With the flaky test, a failure is conclusive, since a commit without the bug never fails. A pass is not. A bad commit passes $r$ runs in a row with probability $(3/4)^r$. In that case the search takes a bad commit for a good one, and goes the wrong way for good. On the worst path, all 10 commits tested are bad, and you need them all to be judged correctly:

Taking the tenth root, $(3/4)^r \le 1 - 0.99^{1/10} \approx 0.001$, so $r \ge \ln(0.001) / \ln(0.75) \approx 24$. Indeed $r = 24$ gives 0.9900 and $r = 23$ gives 0.987. So each bad commit needs a miss rate of about $1/1000$.

For the time, the worst case is 10 commits at 24 runs each: 240 runs, or 24 hours. Every good commit costs all 24 runs, because it has to pass them all. A bad commit stops at its first failure, after about 4 runs on average. A typical path meets 5 of each, so $5 \times 24 + 5 \times 4 = 140$ runs, or 14 hours. A simulation of 4000 searches in the test finds the right commit more than 99% of the time.

In practice, first make the test less flaky, or run it on many machines at once. With 24 runs in parallel, each step takes 6 minutes and the whole search an hour.

Answer to Question 9.29.

I would ask for a change. ReplayBook breaks what Book promises, since any code handed a Book may call add, and here that raises. Split the interface: a read-only view with total, and a mutable Book built on top of it.

The test for inheritance is whether the subclass can stand in everywhere the base is used. This is the Liskov substitution principle, and top_up shows the failure: with a Book it returns a total, and with a ReplayBook it raises. Nothing in the types warns the author of top_up, so the failure shows up at run time, probably in the path nobody tests, and only for replayed books.

The direction of the hierarchy is wrong too. Read-only is the smaller interface, so it should be the base. ReadableBook has total; Book adds add. ReplayBook is then a ReadableBook, and top_up declares that it needs a Book. With a static checker or a C++ compiler, handing it a replayed book is an error at the point of the mistake. In Python you would state this with typing.Protocol and let a type checker enforce it.

Often you should go further and drop inheritance. Replay needs a loader that fills a book and then hands out a read-only view. Composition gives you that with no new class of books at all, and the same Book implementation stays the only one to test.

There is a case for accepting the subclass: a short-lived script, one caller, under a deadline. Say why, and leave a note. I would also accept it if the base class’s contract already declares the operation optional, and callers are written to expect that. But then that contract is the design to question.

Answer to Question 9.30.

new Stats leaves count and sum uninitialised, and reading them is undefined. Tests pass because fresh heap memory happens to be zero. Run with MALLOC_PERTURB_=165: it then prints 1. Write new Stats{}.

Stats has no constructor, so new Stats does default initialisation, which for a plain struct does nothing. new Stats{} does value initialisation, which zeroes it. The two differ by two characters, and neither gets a warning under -Wall -Wextra -Werror, since the allocation and the use are in different functions.

Why does the test pass? In a short-lived process, the allocator hands out memory fresh from the operating system, and that is zeroed. In a long-lived process, the allocator reuses blocks that were freed, so you see old contents, which is the nonsense in production. So the unit test and production really do have different initial conditions.

glibc gives you a way to expose this with no recompile. The man page for mallopt describes M_PERTURB: bytes of allocated memory are initialised to the complement of the value’s low byte. Setting the environment variable MALLOC_PERTURB_=165 fills allocations that come from the main allocator with 0x5a, and the program then prints 1: both fields start at the same huge value, and adding 600 and 3 hardly changes the ratio. With new Stats{}, the same run prints 200. On glibc 2.35, a block reused from the per-thread cache keeps what it had, so to catch every one also set GLIBC_TUNABLES=glibc.malloc.tcache_count=0. The check needs no rebuild, so it can run in every test job.

Other tools exist. g++ 11.4 rejects -fsanitize=memory, as the test confirms. Clang’s MemorySanitizer is the sanitizer built for this, and it needs a rebuild under Clang. Valgrind’s Memcheck reports the use of uninitialised values too. Neither was run for this answer.

Answer to Question 9.31.

The memory is still reachable: done is a global that points to every order. LeakSanitizer reports only memory with no live pointer left at exit, so it says nothing. The three causes are a true leak, unbounded growth of live data, and allocator retention or fragmentation.

The test shows the difference. The listing, run under ASan with 100000 orders, reports nothing. Replace the map insert by a bare new Order whose pointer is thrown away, and it reports the leak at once. The tool answers one exact question, “is anything unreachable?”, and here it is correct. The fault lies in the design: this is what people call a logical leak.

Tell them apart in this order. First, record the allocator’s own view over time alongside the resident size. glibc’s mallinfo2 reports allocated space and free space. Add uordblks and hblkhd for the allocated bytes, since large blocks are mapped directly and counted apart.

If allocated bytes grow with resident memory, you have real growth in use, a leak or a grow-only structure. A heap profiler then gives you the allocation sites, ranked by bytes still live. A site that keeps climbing is your suspect, and you read what holds it. If LeakSanitizer is silent, it is reachable, so look for maps, caches, queues and logs with no bound.

If allocated bytes are flat and resident memory grows, there are two cases. Either free space inside the allocator is growing, or the memory never came through malloc: direct mappings, thread stacks and libraries with their own allocators do not appear at all, so check /proc/PID/smaps. In the first case the allocator is holding freed pages it cannot return, usually because of a mix of sizes that fragments its arenas, or many threads with separate arenas. Look at the free-space figure, and test with another allocator or a bounded arena.

For this code: each entry costs 136 bytes of payload (8 for the key and 128 for the order), plus the tree node’s overhead. Two million fills a day is at least 272 MB a day, which matches the growth. Write the audit to disk, keep only the session’s records in memory, and add a metric for the map’s size.

Answer to Question 9.32.

It is not correct. The first read of instance is unsynchronised against the write in another thread, so this is a data race and undefined behaviour. x86-64 does not save it. The simple correct version is a function-local static. Keeping the double check needs std::atomic with acquire and release.

The classic failure: thread A runs new Config, which is three steps, allocate, construct, assign. Nothing stops the assignment from becoming visible to thread B before the construction, so B sees a non-null pointer and reads a limit that is not yet set. B never takes the lock, so the lock cannot help.

The strong hardware memory model of x86-64 keeps stores in order, but the language’s rules bind the compiler, and it is entitled to reorder a non-atomic write, or to assume no other thread is looking. And the program is undefined regardless of what the machine does. TSan reports data race on this listing in the test.

The simplest fix, also from the test:

Config* get() { static Config instance; return &instance; }

The standard guarantees this: if control enters the declaration concurrently while the variable is being initialised, the other threads wait for it to finish. g++ does it with a guard variable that is checked on every call and __cxa_guard_acquire on the slow path, so it is itself a double check, written for you. TSan is silent.

To keep the pattern, use std::atomic<Config*>. The fast path does an acquire load. The slow path holds the mutex, reloads with relaxed order, constructs, and does a release store. Then a thread that sees the pointer also sees the object, and TSan is silent on this version too. std::call_once is the library form.

In Java, the same pattern works if the field is volatile: the Java Language Specification says that a write to a volatile field happens-before every subsequent read of it. Without volatile it is broken in the same way. That rests on the specification, since no test here can force the reordering on a JVM.

Answer to Question 9.33.

With the object files in the order main, rates, limits, it prints 1000. With main, limits, rates, it prints 0. This is the static initialisation order problem. Use a function-local static, and mark a global constinit to make it a compile error.

Both globals need a call or a read at start-up, so they have dynamic initialisation. Within one file, the order is the order of definition. Across files the standard leaves it indeterminately sequenced, so the order is up to the implementation, and with g++ and GNU ld here it followed the link line.

Before any dynamic initialisation runs, all globals are zero-initialised. So when limits runs first, it reads base_rate as 0, and computes $0 \times 10$. Nothing is uninitialised, no sanitizer complains, and the risk limit is simply zero. Later base_rate becomes 100, as it should, and the wrong value stays in limit. It can get worse: a change in a build script far from either file can flip it.

The fix turns the global into a function:

long& base_rate() { static long v = compute_base(); return v; }

The first caller triggers the initialisation, whichever file it is in, and the test prints 1000 for both link orders.

constinit is the other tool, new in C++20. It requires that a variable be initialised at compile time, and the standard makes the program ill-formed if it has dynamic initialisation. constinit long limit = base_rate * 10; fails to compile, which is what you want: it turns a silent ordering bet into an error. It does not fix anything by itself. It tells you which variables to rewrite.

The most robust design has no such globals. Read the configuration once in main, build the objects in order, and pass them where they are needed. That makes the order a line of code that you can read.

Answer to Question 9.34.

by_virtual and by_variant contain an indirect call or jump. by_final and by_template inline to two lines (plus an endbr64 on Ubuntu’s default flags), and by_switch is compares and branches, with no indirect call. The surprise is by_variant: many people assume std::visit avoids an indirect call.

by_virtual loads the virtual table and ends in jmp *: the compiler cannot know the type. by_final takes a Momentum, which is final, so there is no override to look for, and it inlines. by_template knows the type at compile time, and the test finds identical code to by_final.

On libstdc++ 11, std::visit builds a table of function pointers indexed by the alternative, then uses call *(%rdx,%rax,8). The test reads this out of the assembly. So in this version it is a virtual call in other clothes, plus a check for the valueless state. by_switch tests the index with std::get_if, and everything inlines into two compares and two branches, with a cold path that throws. A later library may compile std::visit differently, so read your own compiler’s output, as this test does, before believing either claim.

For speed, the instruction itself is rarely the main cost. The larger loss is that the compiler cannot inline through an indirect call or optimise across it. Measure it in place.

Other things usually decide the design:

Who can add a type? If strategies arrive from other teams and plugins, the set is open, and virtual is the right tool. Adding a type then touches no existing code. If the set is closed and known, a variant lets the compiler check that every operation handles every type.

Where does it live? A variant is as big as its largest alternative, stored in place with no allocation. That suits a vector of messages. Virtual objects mostly need the heap, and pointer chasing costs more than the call.

Templates give the best code when the type is fixed at build time. But they spread through headers, slow the build and cannot be chosen by a configuration file at run time.

Answer to Question 9.35.

It prints 40. It should print 70, since the resend is the same fill. Make on_fill idempotent by remembering the highest sequence number applied per session, and skip anything at or below it. A gap must stop the keeper.

The fault is that on_fill treats every message as a new event. On any connection that resends after a reconnect, delivery is at least once, not exactly once, so the receiver must make repeats harmless. Listing 9.43 has the shape.

class Position:
    def __init__(self):
        self.qty = 0
        self.last_seq = {}        # session -> the highest sequence number applied

    def on_fill(self, session, fill):
        if fill["seq"] <= self.last_seq.get(session, 0):
            return                # already applied: a resend changes nothing
        if fill["seq"] != self.last_seq.get(session, 0) + 1:
            raise RuntimeError("gap: stop and recover before going on")
        self.qty += fill["qty"] if fill["side"] == "B" else -fill["qty"]
        self.last_seq[session] = fill["seq"]

Three decisions matter here. First, the sequence is per session, so the state is a map, and the test confirms that two sessions do not interfere. Second, a gap raises an error. Silently applying fill 4 after fill 2 hides a missing fill, and the position is then wrong in a way nobody notices. A trading system should stop quoting and ask for the missing range. Third, the check and the update go together, so the state moves only when a fill is applied.

A high-water mark needs the fills to arrive in order on each session, and it stores one number. If order is not guaranteed, or sequence numbers restart, key on a unique execution id instead and remember the ids you have seen. That set grows, so it needs trimming, say at the end of the session. Whichever you use, the mark has to survive a restart along with the position. Save both together, in one atomic write, or rebuild both from the log. If you save the position without the mark, the first resend after a restart counts twice again.

For the tests, take a random fill sequence and deliver it with random resends of the last few messages between new ones. The final position must equal the plain sum of the fills, whatever the resend pattern. The test runs that 200 times. Add a test for a gap, and a test that restarts in the middle.

Answer to Question 9.36.

It sends one order. It should send five, since the ticks are 2 seconds apart in market time. The strategy reads the machine’s clock. Inject the clock and the gateway, so the strategy never finds out which world it is in.

time.time() is hidden input. During a replay, all five ticks arrive within microseconds of wall-clock time, so the 1-second throttle blocks the other four. Live, the same code sends all five. A backtest of this would be wrong in a way that is hard to see, because it runs fine and gives numbers. It would also change with the speed of the machine.

Listing 9.44 has the same logic, except that it asks self.clock.now(), and it sends through whatever gateway it is given. In a backtest, the replay loop sets the simulated clock to each event’s timestamp before delivering the event. Live, the clock wraps the system clock and the gateway sends real orders. The test replays the same ticks and gets all five orders, twice, identically.

class Throttled:
    """The same logic: the time and the way out are given to it."""

    def __init__(self, clock, gateway, level):
        self.clock, self.gateway, self.level = clock, gateway, level
        self.last = float("-inf")

    def on_tick(self, price):
        now = self.clock.now()
        if price > self.level and now - self.last >= 1.0:
            self.last = now
            self.gateway.send(price)


class SimClock:
    """In a backtest the replay loop moves this clock to each event's timestamp."""

    def __init__(self):
        self.t = 0.0

    def now(self):
        return self.t

Note the fixed version’s start value too: self.last is minus infinity. In the original it was 0.0, which works only because time.time() is a large number. A simulated clock starting at 0 would have blocked the first order, and that bug appears only with the new design.

Other hidden inputs break repetition: random numbers without a saved seed; iteration over a Python set, whose order for strings changes between runs; threads, whose interleaving changes the order of events; reading the current date, or a file that changes, or a price from a live service. Order of events with equal timestamps needs a stable rule too.

The test of the whole design is simple: run the same input twice and compare every output, byte for byte. Make that a standing test. In C++, the injection can be a template parameter, so the live path pays nothing for it.

Answer to Question 9.37.

Generate random adds and cancels, keep a plain model of the live orders beside the book, check invariants after every step, and shrink a failure by deleting operations. It comes down to two operations: add order 5 at price 100, then cancel order 5. The bug: cancel leaves a level at quantity 0, so best returns a price nobody is quoting.

Listing 9.45 is all of it, in plain Python with random. It uses no library. Each run draws 30 operations over a small range of ids and prices, because small ranges make collisions likely, and collisions are where bugs live. The model is a dict of live orders. It is simple enough to be obviously right, and that is the point. After each step, two facts must hold: the best price equals the highest price among live orders, and the sum over the levels equals the sum over the live orders.

def check(make_book, runs=500, steps=30, seed=1):
    """Random operations against the plain truth. Returns a short failing list, or None."""
    rng = random.Random(seed)
    for _ in range(runs):
        ops = [(rng.choice("ac"), rng.randint(1, 5), rng.randint(99, 101), rng.randint(1, 9))
               for _ in range(steps)]
        if failure(make_book, ops):
            return shrink(make_book, ops)
    return None


def failure(make_book, ops):
    """Run the operations and check every invariant after each one. True if any fails."""
    book, live = make_book(), {}
    for kind, oid, price, qty in ops:
        if kind == "a" and oid not in live:
            book.add(oid, price, qty)
            live[oid] = (price, qty)
        elif kind == "c" and oid in live:
            book.cancel(oid)
            del live[oid]
        if book.best() != max((p for p, _ in live.values()), default=None):
            return True
        if sum(book.levels.values()) != sum(q for _, q in live.values()):
            return True
    return False


def shrink(make_book, ops):
    """Drop operations one at a time for as long as the failure survives."""
    i = 0
    while i < len(ops):
        shorter = ops[:i] + ops[i + 1:]
        if failure(make_book, shorter):
            ops = shorter
        else:
            i += 1
    return ops

Shrinking is simple too: try removing each operation in turn, and keep the removal if the failure survives. That turned 30 operations into the 2 quoted above, which you can read in one glance. Fixing it means deleting the level when it reaches zero. After that, 1000 runs find nothing. The seed is fixed, so a failure can always be replayed.

The generator skips adds of a live id, so it says nothing about that case, and Book mishandles it: add(1, 100, 5), add(1, 101, 5) leaves 5 lots stranded at 100. Either reject a duplicate id, or let the test generate one and specify the result.

Why did the unit tests miss it? test_cancel_lower cancels the lower of two prices, and only the best is checked, so an empty lower level does not show. Its author tested what they had thought of. A random test has no such preference.

The hard part is choosing the invariants, not generating the inputs. Good ones: compare with a slow model, check that a round trip restores the state, and check conservation of quantity. A library such as Hypothesis does the generation and shrinking better for you, and a firm that already has it should use it. In an interview, showing that you can write the 30-line version counts for more.

Answer to Question 9.38.

Add the field as optional with a default, and upgrade every reader first, then the writers. Retire the old field in the reverse order: stop all readers from needing it, stop the writers from sending it, and then mark its number as never to be reused. Keep decoders for old recordings for as long as you keep the recordings.

Order matters because you cannot flip everything at once, so at each step every reader must handle every message then on the bus. Rollout for adding:

• Change the schema, and give the new field a default that means “not known”.
• Deploy all six readers. They accept old and new messages, and use the default when the field is absent.
• Deploy the writers, which start filling the field.
• Only then write logic that relies on the field being there.

For removal, run it backwards: readers stop using the field, then writers stop sending it, then the schema marks it retired.

The format must let an old reader skip what it does not know, and a new reader find out what is absent. A tagged format gives you this directly. The Protobuf documentation states that old binaries simply ignore a new field when parsing, and that removing a field is safe as long as its number is not used again. That is the rule that matters: reusing a number makes decoding ambiguous, and its best-practice guide says to reserve deleted numbers and never to add a required field.

Formats with a fixed layout, common on latency-sensitive paths, need more care. A field can only be added at the end of a block, because anywhere else it moves the offset of every later field. The message header then needs a version, or a length so that readers can skip what they do not understand. The field’s meaning and units are part of the contract too. Changing cents to ticks under the same field is the worst case, since every decoder keeps working and every number is wrong. Add a new field for that instead.

Tests to add. Keep a set of recorded messages from each past version in the repository, and decode them in every build. Have the build compare the schema with the previous one, and fail on an incompatible change. During the rollout, run old and new decoders side by side on live traffic and compare their outputs.

Recorded data changes the time horizon. A replay in five years must still read today’s messages. So schemas are never deleted, the version travels with the data, and a change that seems safe for live traffic can still break research.

Answer to Question 9.39.

There are four bugs: a dangling pointer after a second order at a level, an erase inside a range-for, the wrong end of the book for bids, and a cancel that leaves its level behind. ASan catches only the first. The second is a memory bug that it misses, and the last two are logic that no memory tool can see.

Dangling pointer. index_ stores &level.back(), a pointer into a std::vector, and the next push_back at that level may reallocate. Sequence: add(100, 1, 5), add(100, 2, 5), cancel(1). ASan reports heap-use-after-free in cancel.

Erase inside the loop. take erases the level from levels_ inside the range-for that walks levels_, so the loop’s iterator is invalid when it steps. Sequence: add(100, 1, 5), add(101, 2, 5), take(7). Here ASan says nothing: the step is in _Rb_tree_increment, compiled inside the uninstrumented libstdc++.so, and the freed node still holds good pointers. It prints the right fill. -D_GLIBCXX_DEBUG stops it, with a message about a singular iterator. So one tool is never enough.

Wrong side. The map orders prices upward, so begin() is the lowest. For asks that is the best. For bids it is the worst. Sequence on a bid side: add(100, 1, 5), add(101, 2, 5), take(3). It fills at 100 and not at the bid of 101. No tool finds this: only a test with an expected answer does. The comment on the class is the clue.

Stale level. cancel sets the quantity to zero and leaves the entry. After add(100, 1, 5), cancel(1), best() returns 100 with nothing there. A take erases each spent level it visits, so empty entries pile up only deeper in the book, where no take reaches, and there they use memory for good.

Listing 9.46 fixes all four. A std::list per level keeps iterators stable. Each index entry stores its price and iterator, so a cancel is $O(1)$ plus the level lookup. The side is a template parameter, so a bid side uses std::greater. take advances with the iterator that erase returns. The test checks it under ASan on 30 random scripts for each side, against a slow Python model.

template <class Better>                      // std::less for asks, std::greater for bids
class Side {
public:
    void add(long price, long id, long qty) {
        auto& level = levels_[price];
        level.push_back({id, qty});
        index_[id] = {price, std::prev(level.end())};   // list iterators survive growth
    }
    bool cancel(long id) {
        auto it = index_.find(id);
        if (it == index_.end()) return false;
        auto level = levels_.find(it->second.price);
        level->second.erase(it->second.where);
        if (level->second.empty()) levels_.erase(level);
        index_.erase(it);
        return true;
    }
    long take(long qty) {
        long filled = 0;
        for (auto it = levels_.begin(); it != levels_.end() && filled < qty;) {
            auto& level = it->second;
            while (!level.empty() && filled < qty) {
                Order& o = level.front();
                const long n = std::min(o.qty, qty - filled);
                o.qty -= n;
                filled += n;
                if (o.qty == 0) { index_.erase(o.id); level.pop_front(); }
            }
            it = level.empty() ? levels_.erase(it) : std::next(it);
        }
        return filled;
    }
    bool empty() const { return levels_.empty(); }
    long best() const { return levels_.begin()->first; }

private:
    struct Where { long price; std::list<Order>::iterator where; };
    std::map<long, std::list<Order>, Better> levels_;
    std::unordered_map<long, Where> index_;
};

On design, I would say four more things. A long for both price and quantity allows them to be swapped, so use types. There is no check on duplicate ids or non-positive quantities. A list allocates on every order, so a hot path would use a pool or an intrusive list. And a real book needs an interface for a fill’s details, not only how much was filled.

Answer to Question 9.40.

Most serious first: a hidden global that mixes files, one bad row killing the whole run, a crash on zero quantity, floats for money, and a file never closed. The mutable default looks bad, but it is harmless here. Accept a pure function over parsed trades, plus a thin file wrapper.

Global state. totals survives between calls. Load day 1 (10 at 100 and 30 at 102, VWAP 101.5), then day 2 (10 at 200), and vwap("AAA") gives 121.2, mixing the two days. No error appears. This is the worst fault, because the number looks plausible. It also means the function cannot be tested without clearing a global first.

No handling of bad rows. One row with price equal to oops raises ValueError, and the file is lost at that row. So a single bad print loses a day. Worse, the earlier rows are already in totals, so a caller who catches the error sees partial totals. Decide on a policy: count and report the bad rows.

Zero quantity. Rows of +10 and -10, as in a correction, give ZeroDivisionError in vwap.

Floats. sum([0.1] * 10) is not 1.0, so small errors build up over millions of rows, and then round adds half-to-even behaviour in the report. Prices arrive as exact text, so keep them exact, as Fraction, Decimal or integer ticks.

The file is never closed. open has no with. CPython closes the file when the object is collected, and raises a ResourceWarning, as the test confirms. Nothing in the language promises when that happens. The file handle is a resource, so say when it ends.

Mutable default. symbols=[] is never changed, so it does no harm here, and saying so shows you read it rather than matched a pattern. Still, ask for None or a tuple. in on a list is also linear, so a set would be better. The way it is written, an empty list means “everything”, which is a surprising rule to leave undocumented.

Structure. load opens, parses, filters and accumulates, and report prints. Nothing can be used alone.

Listing 9.47 separates these: parse turns rows into trades and collects errors, vwaps is a pure function, and report only handles the file. The tests cover both days giving their own answers, bad rows counted with their row numbers, whether a word for a price, a short row or a price of 1/0, None for zero quantity, ten trades at 0.1 averaging exactly 0.1, and, on random trades, that shuffling the input and calling a second time change nothing.

import csv
from collections.abc import Iterable, Iterator
from fractions import Fraction
from typing import NamedTuple


class Trade(NamedTuple):
    symbol: str
    price: Fraction
    qty: int


def parse(rows: Iterable[dict], errors: list) -> Iterator[Trade]:
    for n, row in enumerate(rows, 1):
        try:
            yield Trade(row["symbol"], Fraction(row["price"]), int(row["qty"]))
        except (KeyError, ValueError, TypeError, ZeroDivisionError) as e:
            errors.append((n, repr(e)))


def vwaps(trades: Iterable[Trade]) -> dict:
    """A pure function: same trades in, same numbers out, and no hidden state."""
    totals = {}
    for t in trades:
        notional, qty = totals.get(t.symbol, (Fraction(0), 0))
        totals[t.symbol] = (notional + t.price * t.qty, qty + t.qty)
    return {s: (n / q if q else None) for s, (n, q) in totals.items()}


def report(path: str) -> tuple[dict, list]:
    errors = []
    with open(path, newline="") as f:
        return vwaps(parse(csv.DictReader(f), errors)), errors

The fixed version keeps one running pair per symbol, so memory stays small. It does not check for duplicate trades, and in an interview I would ask what the file guarantees before adding that. A NUL byte still stops the csv reader itself on Python 3.10: treat that as a corrupt file, not a bad row.