electric-sql / pglite

Lightweight WASM Postgres with real-time, reactive bindings.
https://pglite.dev
Apache License 2.0
8.32k stars 164 forks source link

Add lock to indexeddb vfs so that the database can only be opened once. #85

Open Neo-Ciber94 opened 5 months ago

Neo-Ciber94 commented 5 months ago

Each time I try to use the IndexedDb I get this error.

this.program: could not access the server configuration file "/pgdata/postgresql.conf": No such file or directory

image

This is the code that trigger it:

"use client";

import { PGlite } from "@electric-sql/pglite";
import { useEffect, useState } from "react";

export default function PgLiteComponent() {
  const [data, setData] = useState<any>("PENDING");

  useEffect(() => {
    (async () => {
      const client = new PGlite("idb://local", { debug: 1 });
      const result = await client.exec("SELECT NOW()");
      setData(result);
    })();
  }, []);

  return (
    <div>
      <h1>PGLite</h1>
      <pre>{JSON.stringify(data ?? "", null, 2)}</pre>
    </div>
  );
}

The app halts, the error cannot be caught, so is not possible to do anything.

samwillis commented 5 months ago

Thanks for the report, could you let me know what React framework and build tooling you are using?

the "use client" is making me think you may be using Next.js and RSC?

Does this happen in all web browsers or only a specific one?

Neo-Ciber94 commented 5 months ago

Thanks for the report, could you let me know what React framework and build tooling you are using?

the "use client" is making me think you may be using Next.js and RSC?

Does this happen in all web browsers or only a specific one?

I test the code on NextJS, RemixJS and Vite+React, all 3 the same error.

I was using Chrome (Windows).

samwillis commented 5 months ago

Could you check if there is an indexeddb via the Chrome dev tools, if there is one delete it and refresh the app. It may be that the state of the database is broken, I'm suspicious that the initdb phase didn't complete when you first ran the code. Alternatively change the name of the database so that it creates a new one.

Neo-Ciber94 commented 5 months ago

Removing or changing the name of the Indexeddb is not working for me either, is also happening in Firefox:

image

Neo-Ciber94 commented 5 months ago

Not sure where the errors is comming from, is also happening on stackblitz:

https://stackblitz.com/edit/react-pglite-bug-rcynzw?file=src%2FApp.tsx

Be aware that this halts my browser.

image

samwillis commented 5 months ago

Hey @Neo-Ciber94

I've found the problem, it's due to React strict mode. Essentially PGlite is initiated twice for the same database, the first time it creates the indexeddb database to store the files, the second time it sees the database is there and so thinks it doesn't have to initiate it. However the first run hasn't completed and so the database doesn't exist.

I commented out the strict mode on this fork: https://stackblitz.com/edit/react-pglite-bug-rcynzw-cwupa2?file=src%2FApp.tsx

It's one of the occasions where you almost certainly don't want strict mode to init the postgres twice as it's such a cpu heavy operation, even for development. I would recommend explicitly only having a single instance of PGlite, example here (and below): https://stackblitz.com/edit/react-pglite-bug-rcynzw-bzsd5x?file=src%2FApp.tsx,src%2Fmain.tsx,src%2Findex.css

We should look at having a lock so that only one instance of the database can be opened at once with indexeddb and throw an error if it can't be acquired. I'm going to use this issue to track that.

import { PGlite } from '@electric-sql/pglite';
import { useState, useEffect } from 'react';

const DATA_DIR = 'idb://somedatabase2';

let client: PGlite;

export default function App() {
  const [data, setData] = useState<unknown>('PENDING');

  useEffect(() => {
    (async () => {
      client ??= new PGlite(DATA_DIR, { debug: 1 });
      const result = await client.exec('SELECT NOW()');
      setData(result);
    })();
  }, []);

  return (
    <div>
      <h1>PGLite</h1>
      <pre>{JSON.stringify(data, null, 2)}</pre>
    </div>
  );
}
Neo-Ciber94 commented 5 months ago

@samwillis You are totally right, looks like useEffect took other victim.

Ensuring the instance is created only once solve the issue for me.

I really appreciate your help, Thank you!

thruflo commented 5 months ago

Perhaps we could provide a PGlite.createOnce() function or similar, which would return the same instance no matter how many times it’s called.

samwillis commented 5 months ago

@thruflo i like that. Ideally we should use a weak map to store the references to previously open databases so they can be cleanly garbage collected without having to explicitly close it to remove any reference.

berzi commented 4 months ago

If a createOnce() is added, would there be any use for the current syntax that allows multiple instances to be created?

Also is there anything to consider to support workers? Maybe we need a createOnceWithWorkers() too? Even though I don't really know why you would ever need an instance on the main thread in the first place—unless there's a good reason it sounds like an antipattern to even allow it.