refactor: move message handling internals to use async/await

[arthurschreiber]

This introduces some initial refactoring around MessageIO/IncomingMessageStream/OutgoingMessageStream.

Namely, the goal is to completely remove IncomingMessageStream/OutgoingMessageStream/Message and replace them with two straightforward functions instead:

• MessageIO.readMessage to read the contents of a message as a stream using an AsyncIterable<Buffer> from a Readable stream.
• MessageIO.writeMessage to write a stream of buffers (generated synchronously via a Iterable<Buffer> or asynchronously via a AsyncIterable<Buffer>) to a Writable stream.

Both these new methods are much simpler compared to the previous IncomingMessageStream/OutgoingMessageStream implementation, both from a logical complexity as well as an implementation complexity point.

They're also both significantly faster than the current implementations. I added benchmarks that try to compare either implementation (benchmarks run using Node.js v18.20.4):

@arthurschreiber ➜ /workspace/benchmarks (arthur/async-await) $ node message-io/incoming-message-stream.js 
message-io/incoming-message-stream.js n=100: 2,913.7299061537133 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/incoming-message-stream.js n=1000: 11,430.989311430583 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/incoming-message-stream.js n=10000: 32,195.67874171715 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/incoming-message-stream.js n=100000: 101,359.99006469377 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)

@arthurschreiber ➜ /workspace/benchmarks (arthur/async-await) $ node message-io/read-message.js 
message-io/read-message.js n=100: 15,410.443287565346 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/read-message.js n=1000: 22,116.38940123348 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/read-message.js n=10000: 70,465.68105003222 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/read-message.js n=100000: 206,620.15053389582 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)

@arthurschreiber ➜ /workspace/benchmarks (arthur/async-await) $ node message-io/outgoing-message-stream.js 
message-io/outgoing-message-stream.js n=100: 5,938.379808083441 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/outgoing-message-stream.js n=1000: 9,312.915449158187 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/outgoing-message-stream.js n=10000: 37,337.954398051086 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/outgoing-message-stream.js n=100000: 80,630.96081259915 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)

@arthurschreiber ➜ /workspace/benchmarks (arthur/async-await) $ node message-io/write-message.js 
message-io/write-message.js n=100: 18,303.00403544633 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/write-message.js n=1000: 15,285.594130026144 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/write-message.js n=10000: 67,938.25567509481 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
message-io/write-message.js n=100000: 127,125.10735604081 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)

The current implementation spends a lot of time setting up new stream objects for each incoming and outgoing message (via the Message class that inherits from PassThrough transform stream), and that causes quite a dent in performance, especially when v8 optimizations haven't kicked in yet.

[codecov]

Codecov Report

❌ Patch coverage is 90.66667% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.16%. Comparing base (9e27684) to head (c7aa07a).

Files with missing lines	Patch %	Lines
src/message-io.ts	90.83%	3 Missing and 9 partials ⚠️
src/connection.ts	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1656      +/-   ##
==========================================
- Coverage   79.92%   79.16%   -0.77%     
==========================================
  Files          90       90              
  Lines        4887     5030     +143     
  Branches      920      938      +18     
==========================================
+ Hits         3906     3982      +76     
- Misses        676      741      +65     
- Partials      305      307       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[arthurschreiber]

@MichaelSun90 @mShan0 Mind taking a look? I only moved the PRELOGIN payload and parsing to use the new methods so far. I think this is probably large enough to warrant merging this before moving more things over - I don't think this will be reviewable if we move more things over inside of this PR.

[arthurschreiber]

@claude Can you do a thorough review of these changes?

[claude]

Claude finished @arthurschreiber's task in 2m 8s —— View job

---

Thorough Code Review

I've completed a comprehensive review of this PR. Overall, this is a well-designed refactoring with solid performance improvements and cleaner abstractions. However, there are some issues that need attention before merging.

---

🔴 Critical Issues

1. Debug Console Statements Left in Production Code

Location: src/connector.ts:84, 90, 93

console.log('looking up addresses for ', options.host);
console.log('lookup failed', err, process.hrtime(startTime));
console.log('looked up addresses for', options.host, process.hrtime(startTime));

Issue: These debug console.log statements should not be in production code. They should either:

• Be removed entirely, OR
• Be converted to use the existing Debug infrastructure

Impact: High - pollutes console output in production environments

---

2. Missing Semicolon (Syntax Issue)

Location: src/connection.ts:3343

const message = MessageIO.readMessage(socket, this.debug)  // <-- Missing semicolon
const iterator = message[Symbol.asyncIterator]();

Issue: Missing semicolon could cause ASI (Automatic Semicolon Insertion) issues. While JavaScript will likely handle this, it's inconsistent with the codebase style.

Impact: Medium - could cause linting failures, inconsistent with code style

---

⚠️ Important Concerns

3. Error Wrapping Complexity

Location: src/message-io.ts:276-295

The error handling during payload iteration is complex:

} catch (err) {
  // If the stream is still writable, the error came from
  // the payload. We will end the message with the ignore flag set.
  if (stream.writable) {
    const packet = new Packet(type);
    packet.packetId(packetNumber += 1);
    packet.resetConnection(resetConnection);
    packet.last(true);
    packet.ignore(true);
    // ... write packet
  }
  throw err;
}

Observation: While this handles the error gracefully by sending an ignore packet, there's a subtle issue: if the stream becomes unwritable between the error occurring and the check, the ignore packet won't be sent. Consider if this is the desired behavior.

Question: Should there be additional error wrapping here similar to the approach taken in connection.ts (see commits "ensure errors are wrapped correctly" and "Don't wrap errors twice")?

---

4. Race Condition in Error Handling

Location: src/message-io.ts:233-240

function onError(err: Error) {
  if (drainReject) {
    const cb = drainReject;
    drainResolve = null;
    drainReject = null;
    cb(err);
  }
}

Observation: The onError handler only calls the reject callback if drainReject is set. If an error occurs when we're not waiting for drain, it's silently ignored because the error listener is set up early in the function (line 244) but might fire before we're waiting.

Impact: Medium - Could potentially miss errors in edge cases

---

5. Commented-out Test Code

Location: test/unit/message-io-test.ts:484-524

There's a large block of commented-out test code for "reads data that is sent across multiple packets".

Recommendation: Either:

• Re-enable this test if it's needed, OR
• Remove it if it's no longer relevant to the new implementation

---

6. Security Warnings (Pre-existing, not introduced by this PR)

Location: Multiple test files

GitHub Advanced Security flagged clear-text logging of sensitive information (passwords) in test files. While these appear to be pre-existing and only in test code when TEDIOUS_DEBUG is enabled, consider whether password logging should be redacted even in debug mode:

• test/integration/bulk-load-test.js:52
• test/integration/datatypes-in-results-test.ts:42
• test/integration/rpc-test.js:54

Note: These are not introduced by this PR but worth addressing.

---

✅ Positive Observations

1. Excellent Performance Improvements

The benchmarks show significant performance gains (2-3x faster in many cases). This is a substantial improvement that will benefit all users.

2. Cleaner Abstractions

The new static methods MessageIO.readMessage() and MessageIO.writeMessage() are much simpler than the previous stream-based approach:

• Reduced complexity (no need for PassThrough streams)
• More predictable behavior with async/await
• Easier to reason about

3. Comprehensive Error Handling

The new implementations handle multiple error scenarios:

• Stream errors during writes (message-io.ts:155-176, 178-207, 209-242)
• Errors during payload iteration (message-io.ts:276-295)
• Premature stream closure (message-io.ts:212-213, 352-354)
• Proper cleanup in finally blocks

4. Excellent Test Coverage

The unit tests comprehensively cover:

• Error handling during iteration
• Error handling during drain
• Premature close scenarios
• Multi-packet messages
• TLS negotiation edge cases

The test coverage at 90.66% is very good for this type of refactoring.

5. Proper Resource Cleanup

Both readMessage and writeMessage properly clean up event listeners in finally blocks:

• message-io.ts:333-337 (writeMessage)
• message-io.ts:447-451 (readMessage)

6. Async Iterator Pattern

The use of async generators for readMessage is elegant and fits well with modern JavaScript patterns:

static async *readMessage(stream: Readable, debug: Debug) {
  // ... yields packet data as it arrives
}

---

📝 Minor Suggestions

7. Type Safety Enhancement

Location: src/message-io.ts:254-260

The type detection logic could be more type-safe:

if ((payload as AsyncIterable<Buffer>)[Symbol.asyncIterator]) {
  isAsync = true;
  iterator = (payload as AsyncIterable<Buffer>)[Symbol.asyncIterator]();
} else {
  isAsync = false;
  iterator = (payload as Iterable<Buffer>)[Symbol.iterator]();
}

Suggestion: Consider using a type guard function for cleaner type checking:

function isAsyncIterable<T>(obj: any): obj is AsyncIterable<T> {
  return obj && typeof obj[Symbol.asyncIterator] === 'function';
}

8. TODO Comments

Location: src/message-io.ts:301, 319

There are TODO comments about getting rid of creating Packet instances:

// TODO: Get rid of creating `Packet` instances here.

Question: Is this planned for a future PR, or should it be addressed in this one?

---

🎯 Summary

Recommendation: This PR should be approved after addressing the critical issues (#1 and #2).

Must Fix Before Merge:

1. Remove or properly implement debug console.log statements in connector.ts
2. Add missing semicolon in connection.ts:3343

Should Consider:
3. Review error handling logic for potential edge cases (issues #3 and #4)
4. Remove or re-enable commented test code
5. Consider type safety improvements

Great Work:

• Performance improvements are substantial and well-benchmarked
• Code is cleaner and more maintainable
• Error handling is comprehensive
• Test coverage is excellent

---

Changed Files Analysis

Core Implementation:

• ✅ src/message-io.ts - Well implemented, minor issues noted above
• ⚠️ src/connector.ts - Contains debug console.log statements (MUST FIX)
• ⚠️ src/connection.ts - Missing semicolon (MUST FIX)

Tests:

• ✅ test/unit/message-io-test.ts - Comprehensive coverage
• ✅ test/integration/* - Only debug logging changes (pre-existing security concerns noted)

Benchmarks:

• ✅ All benchmark files properly implemented

---