search

libp2p / js-libp2p-kad-dht

JavaScript implementation of the DHT for libp2p
https://libp2p.io
Other
141 stars 60 forks source link

Uncatchable errors with new promisify/async support #131

Closed JustMaier closed 5 years ago

JustMaier commented 5 years ago

Since errors are being thrown inside the asynchronous setImmediate methods, they are uncatchable. This leads to various problems, such as uncaught errors when trying to fetch an item from the DHT for the first time because the datastore will throw an error that cannot be caught.

dirkmc commented 5 years ago

@JustMaier thanks for taking a look through the code. Could you point us to a place where this problem occurs?

kumavis commented 5 years ago

Not sure if this is what @JustMaier is experiencing but I noticed if you have a "sometimes bug" in your queryFunc it just gets appended to the run.errors and is easy to miss

JustMaier commented 5 years ago

@dirkmc I think my problem might have actually stemmed from the fact that I was using interface-datastore@0.7.0 which made the switch to async/await and async iterables. Since that version no longer takes callbacks, promise rejections/errors weren't getting caught by the callbacks that were being passed to the datastore.

Initially, after reviewing the codebase, I thought that perhaps the rejections weren't being handled correctly by the callback functions wrapped with promisify, especially when setImmediate was involved. Instead, it was just because the datastore errors weren't being handled because of the change to async.

I started to try and get the library to work with the latest datastore, but struggled with async iterables and fully understanding the tests. Happy to open a PR with the changes I made if you think it's appropriate.

dirkmc commented 5 years ago

@JustMaier we're currently working on updating the DHT to use async/await, but it's being done piece by piece, so probably it would make most sense to wait until that process is complete before upgrading to an async/await version of interface-datastore

JustMaier commented 5 years ago

Sounds good. I hadn't noticed, but it looks like @kumavis is actually in the process of working through the changes now.

Thanks for your guy's hard work on this stuff. It's been awesome to see it get better and better.