Database tests 2db #707
Database tests 2db #707
Conversation
|
there is a random segmentation fault on the unit tests, checking |
apparently its was an issue related with the storage tests, seems fixed now |
|
Are the tests failing due to the changes in this PR or is something else random going on? Other than that it all looks good. |
its bigger than that, its also related with the issue experienced on #551 |
| @@ -85,15 +90,83 @@ export class OceanIndexer { | |||
| return false | |||
| } | |||
|
|
|||
| // it does not start crawling until the network connectin is ready | |||
| async startCrawler(blockchain: Blockchain): Promise<boolean> { | |||
There was a problem hiding this comment.
was there a reason why you moved this to index from the thread, or was just one of the things you tried. Asking from curiosity mainly 😄 ?
There was a problem hiding this comment.
Both :-)
Remember this? #551
When the thread exits we CANNOT process any more code inside, that's what originates the segmentation fault / core dumps
Everything works fine when we have proper DB and RPC connections, but when we don't we get a lot of errors, From DB errors to RPC errors.. Many of these can also cause the thread to exit, and we get that error code 1 on 'exit'..
If we exit the thread and are still trying to connect to a provider (wrong rpc for instance), that is one example.
Either by us retrying the connection (checking if is ready), or the JSONRPCProvider itself, like here:
https://github.com/ethers-io/ethers.js/blob/9e7e7f3e2f2d51019aaa782e6290e079c38332fb/src.ts/providers/provider-jsonrpc.ts#L767
when we create a new JSONRPCProvider it retries at least 4 or 5 times on its own.. We can't have any of this code on the crawler thread. Blockchain connection MUST be already established before starting the thread, same goes for DB
| return false | ||
| } | ||
| } | ||
| // async function retryCrawlerWithDelay( |
There was a problem hiding this comment.
also if we decide to keep in the index lets remove it for real from the thread
| let config: OceanNodeConfig | ||
| before(async () => { | ||
| config = await getConfiguration(true) | ||
| mockDatabase = await new Database(config.dbConfig) |
There was a problem hiding this comment.
with this this test starts to look more like an integration test, not 100% sure if we should keep it or lets say keep it under unit tests 😃 , but nothing critical from my point of view
There was a problem hiding this comment.
not really :-) , its just creating an instance of the class and calling a few public functions, not much different than all the others on the unit test suite
In this particular case, the issue with test was that the mockup functions were not doing anything... only the real indexer code was called...And now with new DB (when the test is actually running on Elastic)
the thread exits because of all the elastic db connections errors. Elastic seems to be more harsh than typesense on the thread itself, i think because of the uncaugh exceptions on "initializeIndex calls") ... These make the thread to exit ....and after it exits ... we still have the JSONRPCProvider code running and trying to connect to a wrong RPC, detect connection status, etc... all inside a thread that no longer exists
Therefore a core dump
I alread suspected of the CrawlerThread running code after it exits, when we were discussing #551 but could not understand exactly what could be... now i'm certain it was this RPC/blockchain fail/retry mechanism. We also have that while(true) with a sleep() in between... which can also be problematic if the thread exits in the middle... but lets not worry much with that atm, as long as we only start the crawling when everything is OK we should be safer
Fixes # .
Changes proposed in this PR:
Configurationobject (avoid constant direct access toprocess.env)(sometimes we had different components running with different DB, on same tests, and some tests only passed because they "caught" a specific DB, but would not run on the other.. it was a bit random, with all the
.envchanges going on)