Fixes a bug where a promise was unhandled. #11
Conversation
Fixes ethjs#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](...)`).
|
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. |
|
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 |
|
Not sure if it helps, but I am getting an And the actual error message never made it out to my server code, where I can actually try to handle it |
|
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 ***@***.***> 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.
|
|
Just a simple |
|
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 ***@***.***> 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.
|
|
@williamchong007 Upgrade to the latest version of @SilentCicero I don't use The fundamental issue here is that if you instantiate a Promise in node, you must hookup a |
|
I dived into my local docker-compose container and checked the ethjs version, Interestingly it does not seems to happen at all, when I deploy the same container onto production k8s |
|
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 ***@***.***> 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.
|
|
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. |
|
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 ***@***.***> 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.
|
|
this was resolved! 😸 |
Fixes #10.
Rather than intermixing callbacks and promises, this opts to just use promises all the way through.
Giant caveat: This means that the
querypassed 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 ofquery[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.