google / lovefield

Lovefield is a relational database for web apps. Written in JavaScript, works cross-browser. Provides SQL-like APIs that are fast, safe, and easy to use.
https://google.github.io/lovefield/
Apache License 2.0
6.82k stars 367 forks source link

Connection fails on DB with 300K+ rows. In some cases Chrome completely crashes #240

Closed Tudmotu closed 5 years ago

Tudmotu commented 6 years ago

Hi there, We love this awesome project and have been using it in one of our products.

We started noticing crashes of our app. In some cases, the DB fails to load, in other cases Chrome crashes completely.

After investigating, it seems that at a certain size of data, Lovefield can no longer make connections to the DB.

I've put together a demo that reproduces on my machine (Arch Linux, Chrome 67, 16GB RAM): https://jsfiddle.net/0td6Leb4/ When clicking "Run", we start writing 300K rows to the DB. After writing we reload the page, and then Lovefield is unable to connect anymore, with the error: 'DOMException: Maximum IPC message size exceeded' (can be seen in the console). Clearing the DB allows Lovefield to connect again.

NOTE: This might crash your Chrome.

The demo tries to resemble our own actual code (~300k rows, ~17 columns/row, PK on all columns)

We're still trying to figure out how to best handle this. This is especially hard when reaches the point of crashing chrome. If anyone has any ideas on how to avoid this or how to handle, we would greatly appreciate it.

freshp86 commented 6 years ago

Hi. The error seems to be coming from IDB itself, see https://codereview.chromium.org/1321583002 which added that error message, and https://cs.chromium.org/chromium/src/content/browser/indexed_db/indexed_db_database.cc?l=1154 for the corresponding code.

I am suspecting that the error happens because of Lovefield using getAll() under the cover. Needs investigation on potential mitigation.

freshp86 commented 6 years ago

So, IIUC by reading the links above, the problem can be mitigated by not using the getAll() method at, https://github.com/google/lovefield/blob/ddce6483511c61f2da191dedde8a7f4d609cb2fd/lib/backstore/object_store.js#L46-L47 Note that Lovefield already has a way to retrieve the DB using a cursor instead (in fact this was the original way of retrieving data on startup), which should circumvent the max IPC message size error.

@arthurhsu Perhaps we can expose a config option in connect() to allow the user to specify whether getAllBulk_() or getAllWithCursor_() will be used.

At the same time, we should probably contact the IDB team and see if they can get an exception within Chromium to exceed the IPC limit, specified at https://cs.chromium.org/chromium/src/ipc/ipc_channel.h?g=0&l=144.

freshp86 commented 6 years ago

FYI filed https://bugs.chromium.org/p/chromium/issues/detail?id=868177 to explore what can be done on the Chromium side.

arthurhsu commented 6 years ago

https://github.com/google/lovefield/commit/37936e449494f3f0340bc62cdfe5ba6fc449514a

This is tested locally and should address the problem for now. I'm out of office until next week and will be on vacation very soon. freshp86@ if you have time tomorrow please help patch and merge this. Thanks!

Tudmotu commented 6 years ago

Oh, nice, thank you both! This was a very quick response :smiley:

After more investigation, it seems the crashing issue is not directly related. Happens when Lovefield is interrupted while writing. For now I will try this fix and keep looking into the crashing.

Thanks!

Tudmotu commented 6 years ago

Update: 37936e4 seems to fix the issue :smiley: It also seems to fix the crash. Still unable to consistently reproduce the crash, but on some machines where it happens consistently, this fix seems to mitigate it.

freshp86 commented 6 years ago

FYI the fixed has been pushed to master, and will be part of the next release.

gutenye commented 5 years ago

@freshp86 @arthurhsu I also hit this and thanks for the fix. When will be next release published? Last one was 2 years ago :(