search

ethjs / ethjs-contract

A simple contract object for the Ethereum RPC layer.
MIT License
20 stars 28 forks source link

Fixes a bug where a promise was unhandled. #11

Closed MicahZoltu closed 6 years ago

MicahZoltu commented 7 years ago

Fixes #10.

Rather than intermixing callbacks and promises, this opts to just use promises all the way through.

Giant caveat: This means that the query passed in to the constructor must support promises. I personally think this is the right/sane thing to do, but if you want to continue to support callbacks you will need to solve this bug in a different way (e.g., swallow the promise result of query[queryMethod](...)).

ethjs-contract

Thank you for contributing! Please take a moment to review our contributing guidelines to make the process easy and effective for everyone involved.

Please open an issue before embarking on any significant pull request, especially those that add a new library or change existing tests, otherwise you risk spending a lot of time working on something that might not end up being merged into the project.

Before opening a pull request, please ensure:

Be kind to code reviewers, please try to keep pull requests as small and focused as possible :)

IMPORTANT: By submitting a patch, you agree to allow the project owners to license your work under the terms of the MIT License.

MicahZoltu commented 7 years ago

I just realized the code I wrote is a modification to the compiled output, not the original source. Recommend closing this PR and having someone who actually has the project checked out locally applying the fix.

SilentCicero commented 6 years ago

Okay, this should not happen, how exactly are errors getting swallowed though? I might want to implement a try/catch in the meantime if that is the case. @MicahZoltu

williamchong commented 6 years ago

Not sure if it helps, but I am getting an UnhandledPromiseRejectionWarning every time something fails with rpc @SilentCicero 

node-server_1  | (node:28) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: [ethjs-rpc]  error with payload {"id":2575583708953,"jsonrpc":"2.0","params":[{"to":"0xA97fEfe489ca62ADD4949360DCE30e3351F6881B","data":"0x8eaa6ac01201cf23b4c1d237a7c146584a8e540995f700cf38525cf627cc3407eac17416"},"latest"],"method":"eth_call"} Error: [ethjs-provider-http] Invalid JSON RPC response from provider
node-server_1  |     host: https://rinkeby.infura.io/ywCD9mvUruQeYcZcyghk
node-server_1  |     response:  ""
node-server_1  |     responseURL: undefined
node-server_1  |     status: 0
node-server_1  |     statusText:
node-server_1  |
node-server_1  | (node:28) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

And the actual error message never made it out to my server code, where I can actually try to handle it

SilentCicero commented 6 years ago

So I just updated the rpc with new code that exposes the rpc errors. Perhaps this is triggering the error here. Can you show more code as to what is triggering this?

Sent from my iPhone

On Dec 5, 2017, at 2:15 PM, William Chong notifications@github.com wrote:

Not sure if it helps, but I am getting an UnhandledPromiseRejectionWarning everytime something fails with rpc @SilentCicero

node-server_1 | (node:28) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: [ethjs-rpc] error with payload {"id":2575583708953,"jsonrpc":"2.0","params":[{"to":"0xA97fEfe489ca62ADD4949360DCE30e3351F6881B","data":"0x8eaa6ac01201cf23b4c1d237a7c146584a8e540995f700cf38525cf627cc3407eac17416"},"latest"],"method":"eth_call"} Error: [ethjs-provider-http] Invalid JSON RPC response from provider node-server_1 | host: https://rinkeby.infura.io/ywCD9mvUruQeYcZcyghk node-server_1 | response: "" node-server_1 | responseURL: undefined node-server_1 | status: 0 node-server_1 | statusText: node-server_1 | node-server_1 | (node:28) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

williamchong commented 6 years ago

Just a simple get() of a smart contract deployed in rinkeby https://github.com/likecoin/likecoin-poc/blob/master/server.js#L155 It fails randomly but I was never able to debug it, due to the error message being empty like above

SilentCicero commented 6 years ago

What version of ethjs are you using? Try the latest npm version. I believe 0.3.1

Sent from my iPhone

On Dec 5, 2017, at 5:19 PM, William Chong notifications@github.com wrote:

Just a simple get() of a smart contract deployed in rinkeby https://github.com/likecoin/likecoin-poc/blob/master/server.js#L155 It fails randomly but I was never able to debug it, due to the error message being empty like above

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

MicahZoltu commented 6 years ago

@williamchong007 Upgrade to the latest version of ethjs-query/ethjs-rpc. It resolves the null error message issue, allowing you to actually debug your code. :)

@SilentCicero I don't use ethjs-contract anymore, but the problem still occurs with ethjs-query and I believe it has the same root cause. To reproduce the problem, cause the JSON-RPC return a JSON-RPC error when calling await this.connector.ethjsQuery.estimateGas(transaction);. This should result in the rejected promise turning into a node exception here due to the await call which can then be wrapped in a try/catch. However, instead what happens is node screams about an unhandled promise rejection. This is because (I believe) that a promise is being generated by one of the lower level libraries but a callback is being used to detect the error instead of the promise.

The fundamental issue here is that if you instantiate a Promise in node, you must hookup a .catch to it OR await it. Throughout the ethjs codebase there is a weird mix of promises + callbacks. If someone uses callbacks to monitor for failure, they will not hookup a .catch to the Promise which means they'll get an unhandled promise rejection error from node. I strongly recommend removing support for callbacks and just using promises. If someone really wants, they can create a wrapper that turns the promise into a callback, but in a way that appropriately sets up the .then and .catch. I believe there are even libraries that do this automatically for you.

williamchong commented 6 years ago

I dived into my local docker-compose container and checked the ethjs version, and above UnhandledPromiseRejectionWarning still happen @0.3.1

Interestingly it does not seems to happen at all, when I deploy the same container onto production k8s

SilentCicero commented 6 years ago

Interesting! I'll look into dropping callbacks. I think that makes a lot of sense now that async await is taking off.

Sent from my iPhone

On Dec 6, 2017, at 7:13 AM, William Chong notifications@github.com wrote:

I dived into my docker-compose container and checked the ethjs version, and above UnhandledPromiseRejectionWarning still happen @0.3.1

Interestingly it does not seems to happen at all when I deploy the same container onto production k8s

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

karvex commented 6 years ago

I don't know if this is a good workaround, but I commented out "reject(callbackError)" from line 8350 and then I could catch the exception without having any problem with unhandled exception.

if (callbackError) {
    //reject(callbackError);
    protoCallback(callbackError, null);
SilentCicero commented 6 years ago

I'll look into this solution. I may just remove callback support altogether as Mike suggested.

Sent from my iPhone

On Dec 22, 2017, at 6:29 PM, Chi-Hao Poon notifications@github.com wrote:

I don't know if this is a good workaround, but I commented out "reject(callbackError)" from line 8350 and then I could catch the exception without having any problem with unhandled exception.

if (callbackError) { //reject(callbackError); protoCallback(callbackError, null); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kumavis commented 6 years ago

this was resolved! 😸