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

Improving connect promise handling #134

Merged
merged 2 commits into from
Jun 15, 2019
Merged

Improving connect promise handling #134

merged 2 commits into from
Jun 15, 2019

Conversation

palavrov
Copy link
Contributor

@palavrov palavrov commented Jun 6, 2019

A buggy device was closing the socket immediately without sending any data to be parsed so the promise was never resolved or rejected i.e. effectively deadlocking i.e. endless await telnetClinet.connect()

This PR fixes just connect method but probably other methods (send, exec, etc) needs to be fixed too.

Generally the problem comes because the underlying socket API is event based and doesn't go well with promises. Another problem comes due lack of control to socket.timeout i.e. we can't cancel it. In overall error handling is really messy and unstable. I did my best with this patch to improve but probably will be better to rewrite everything in a better way.

@mkozjak
Copy link
Owner

mkozjak commented Jun 6, 2019

Hey, @palavrov, thanks for the PR. Please check if the tests pass. Something with tests exit code is not right currently so the travis seems to pass, but has stderr. Sorry about that! #135

@palavrov
Copy link
Contributor Author

palavrov commented Jun 6, 2019

Everything seems OK.

The test warns but there is nothing wrong because it is using event based API not the promise one i.e. there is no one to consume the rejected promise which is wrong but totally expected by design.

Probably it was just luck that non handled rejections didn't occur early in the tests ... I got them all the time with telnet-client.

As said above - mixing events with promises needs better implementation.

I think we have several choices:

  • to adapt the tests to handle the promise rejection
  • to try to guess run time when the connection is event mode and to avoid all promises ... that will be ugly and may be not possible at all
  • to separate the implementation to event based and promise based - IMHO the best way to go but will needs significantly more efforts

@palavrov
Copy link
Contributor Author

palavrov commented Jun 6, 2019

Seems that it'll be possible to detect when someone doesn't use the promise to prevent rejections but it will be really ugly. Bluebird has nice feature for promise monitoring so we could use promiseChained event to know when someone uses .then() and probably await ... but really don't like such quick'n'dirty approach

@mkozjak
Copy link
Owner

mkozjak commented Jun 12, 2019

@palavrov Ok, looks good for now! Can you please remove semicolons and add some newlines before your if statements? Just general code convention. Merging after that one.

@palavrov
Copy link
Contributor Author

Done!

Copy link
Owner

@mkozjak mkozjak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mkozjak mkozjak merged commit eecd60d into mkozjak:master Jun 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants