Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

64 changes: 53 additions & 11 deletions src/core/sqlite-db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { applyMergePatch } from './json-utils';
interface WasmDatabaseInstance {
exec(sql: string, params?: unknown[]): Array<{ columns: string[]; values: unknown[][] }>;
prepare(sql: string, params?: unknown[]): any;
iterateStatements(sql: string): IterableIterator<any>;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

For better type safety and maintainability, you can use the Statement type from sql.js instead of any. This will allow TypeScript to correctly infer types for variables that use this iterator, such as stmt in the executeQuery method.

Suggested change
iterateStatements(sql: string): IterableIterator<any>;
iterateStatements(sql: string): IterableIterator<import('sql.js').Statement>;

export(): Uint8Array;
close(): void;
}
Expand All @@ -57,10 +58,12 @@ interface WasmEngineModule {
*/
class WasmDatabaseEngine implements DatabaseOperations {
private readonly instance: WasmDatabaseInstance;
private readonly queryTimeout: number;
readonly engineKind = Promise.resolve('wasm' as const);

constructor(instance: WasmDatabaseInstance) {
constructor(instance: WasmDatabaseInstance, timeoutMs: number) {
this.instance = instance;
this.queryTimeout = timeoutMs;
}

/**
Expand All @@ -74,20 +77,59 @@ class WasmDatabaseEngine implements DatabaseOperations {
* @returns Array of result sets in sql.js format
*/
async executeQuery(sql: string, params?: CellValue[]): Promise<QueryResultSet[]> {
const startTime = Date.now();
const results: QueryResultSet[] = [];
let currentStmt: any = null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To improve type safety, currentStmt can be strongly typed as import('sql.js').Statement | null instead of any. This complements the change to iterateStatements to return a typed iterator.

Suggested change
let currentStmt: any = null;
let currentStmt: import('sql.js').Statement | null = null;


try {
const rawResults = this.instance.exec(sql, params);
// Return in sql.js compatible format for webview compatibility
return rawResults.map(resultSet => ({
columns: resultSet.columns,
values: resultSet.values as CellValue[][],
// Also provide our new format for internal use
headers: resultSet.columns,
rows: resultSet.values as CellValue[][]
})) as QueryResultSet[];
const iterator = this.instance.iterateStatements(sql);
let isFirstStatement = true;

for (const stmt of iterator) {
currentStmt = stmt;

// Bind parameters only to the first statement to match exec behavior
if (isFirstStatement && params && params.length > 0) {
stmt.bind(params);
}
isFirstStatement = false;

const rows: CellValue[][] = [];

while (stmt.step()) {
// Check timeout
if (Date.now() - startTime > this.queryTimeout) {
stmt.free(); // Free current statement to prevent leaks
throw new Error(`Query execution timed out after ${this.queryTimeout}ms`);
Comment on lines +102 to +103
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There's a potential for a double-free issue here. When a timeout occurs, stmt.free() is called, and an error is thrown. The catch block will then attempt to call currentStmt.free() again on the same statement object, as currentStmt still holds the reference and is not nullified. While the empty catch in the error handler would suppress a crash, it's safer to prevent the double-free attempt altogether.

By setting currentStmt to null after freeing it, you ensure the cleanup logic in the catch block won't try to free it again.

             stmt.free(); // Free current statement to prevent leaks
             currentStmt = null; // Avoid double-free in the catch block
             throw new Error(`Query execution timed out after ${this.queryTimeout}ms`);

}
rows.push(stmt.get());
}

const columns = stmt.getColumnNames();
// Only include results that have columns (matching exec behavior)
if (columns.length > 0) {
results.push({
columns,
values: rows,
headers: columns,
rows
});
}

// iterateStatements handles freeing the statement when we proceed to next or finish
// but we clear our reference so we don't try to free it in catch block
currentStmt = null;
}
} catch (err) {
// Ensure current statement is freed if iteration was interrupted by error/timeout
if (currentStmt) {
try { currentStmt.free(); } catch {}
}
const errorDetail = err instanceof Error ? err.message : String(err);
throw new Error(`Query failed: ${errorDetail}`);
}

return results;
}

/**
Expand Down Expand Up @@ -777,7 +819,7 @@ export async function createDatabaseEngine(
wasmInstance = new SqlJsModule.Database();
}

const engine = new WasmDatabaseEngine(wasmInstance);
const engine = new WasmDatabaseEngine(wasmInstance, config.queryTimeout || 50000);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The default query timeout 50000 is a magic number. It's better to define it as a named constant to improve readability and make it easier to change in the future. For example: const DEFAULT_QUERY_TIMEOUT_MS = 50000;.

  const DEFAULT_QUERY_TIMEOUT_MS = 50000;
  const engine = new WasmDatabaseEngine(wasmInstance, config.queryTimeout || DEFAULT_QUERY_TIMEOUT_MS);


return {
operations: engine,
Expand Down
2 changes: 2 additions & 0 deletions src/core/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,8 @@ export interface DatabaseInitConfig {
wasmBinary?: Uint8Array;
/** Open in read-only mode */
readOnlyMode?: boolean;
/** Query execution timeout in milliseconds */
queryTimeout?: number;
}

/**
Expand Down
42 changes: 42 additions & 0 deletions tests/unit/wasm-timeout.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@

import { test } from 'node:test';
import assert from 'node:assert';
import { createDatabaseEngine } from '../../src/core/sqlite-db';

test('WasmDatabaseEngine timeout test', async (t) => {
await t.test('should timeout on long running query', async () => {
const { operations } = await createDatabaseEngine({
content: null,
maxSize: 0,
queryTimeout: 100 // 100ms timeout
});

// Recursive CTE that generates infinite rows
const sql = `
WITH RECURSIVE cnt(x) AS (
SELECT 1
UNION ALL
SELECT x+1 FROM cnt
)
SELECT x FROM cnt;
`;

await assert.rejects(async () => {
await operations.executeQuery(sql);
}, {
message: /Query execution timed out after 100ms/
});
});

await t.test('should run fast query successfully', async () => {
const { operations } = await createDatabaseEngine({
content: null,
maxSize: 0,
queryTimeout: 100
});

const result = await operations.executeQuery("SELECT 1 as val");
assert.strictEqual(result.length, 1);
assert.strictEqual(result[0].rows[0][0], 1);
});
});