Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document "CoapClient was reset" promise rejection #46

Closed
AlCalzone opened this issue Mar 11, 2018 · 10 comments
Closed

Document "CoapClient was reset" promise rejection #46

AlCalzone opened this issue Mar 11, 2018 · 10 comments

Comments

@AlCalzone
Copy link
Owner

see #44 (comment)

@AlCalzone AlCalzone added the bug label Mar 11, 2018
@AlCalzone AlCalzone self-assigned this Mar 11, 2018
@AlCalzone AlCalzone changed the title Handle "CoapClient was reset" promise rejection Document "CoapClient was reset" promise rejection Mar 11, 2018
@AlCalzone AlCalzone added enhancement and removed bug labels Mar 11, 2018
@AlCalzone
Copy link
Owner Author

This can happen on any request/observation that has not completed yet when the user calls reset(). It's not a critical error but has to be there to notify the user that his request cannot be completed.

@neophob
Copy link
Contributor

neophob commented Mar 12, 2018

for reference, this error I captured:

Error: CoapClient was reset
    at normalizeReason (/myproject/node_modules/node-coap-client/build/lib/DeferredPromise.js:5:16)
    at Promise.rej [as reject] (/myproject/node_modules/node-coap-client/build/lib/DeferredPromise.js:13:36)
    at Function.reset (/myproject/node_modules/node-coap-client/build/CoapClient.js:191:33)
    at TradfriClient.reset (/myproject/node_modules/node-tradfri-client/build/tradfri-client.js:158:39)
    at tradfriClient.ping.catch.then (/myproject/app/lib/tradfriService.js:167:32)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

To reproduce:

  • make sure you can control your lights
  • disconnect ethernet not tradfri gateway
  • send 10 commands (for example turnOn) to the gateway
  • wait a minute, then the error pops up

@neophob
Copy link
Contributor

neophob commented Mar 12, 2018

Another option would capture this rejected promise and use the error callback to signal an error

@AlCalzone
Copy link
Owner Author

The error pops up, because you call reset in your app:

at TradfriClient.reset (/myproject/node_modules/node-tradfri-client/build/tradfri-client.js:158:39)
at tradfriClient.ping.catch.then (/myproject/app/lib/tradfriService.js:167:32)

@neophob
Copy link
Contributor

neophob commented Mar 12, 2018

I know, it was just an idea, either update the docs that reset is an async function that returns a promise OR don't change the docs but use the error callback to signal the error.

@AlCalzone
Copy link
Owner Author

I'm leaning towards emitting a general library error and swallow the rejections. This means the corresponding documentation will have to state that all pending promises will neither be resolved or rejected anymore. Do you think this is good from a user's point of view?

Regarding your code snippet:
Are you catching rejections from ping? IMO that should always resolve and never reject. Or am I misunderstanding your stack frame?

@neophob
Copy link
Contributor

neophob commented Mar 12, 2018

I'm fine with swallowing the error and emitting an error - i think as long as the emitted error is understandable it should be fine and makes the implementation easier.

Yes I catch ping call, there was a rejection when I rebooted the gateway.

@AlCalzone
Copy link
Owner Author

Yes I catch ping call, there was a rejection when I rebooted the gateway.

DTLS timeout? If so, that's a bug since ping should handle the errors itself.

@AlCalzone
Copy link
Owner Author

"CoapClient was reset" should now be swallowed and transformed into an "error" event with a slightly more descriptive message.

@AlCalzone
Copy link
Owner Author

Released in v0.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants