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

fix: emit error on timeout request and disconnect #42

Closed

Conversation

olzzon
Copy link

@olzzon olzzon commented Nov 27, 2024

About the Contributor

This PR is made on behalf of TV2 NO

Type of Contribution

This is a:
Bug fix

Current Behavior

Currently disconnection and timeout of request could cause an unhandled crash, that couldn't be caught by a try-catch in the main program.

New Behavior

disconnection and request issues (timeout & wrong request) now emit's an error for the main program to handle.

Testing Instructions

Connect an Ember client to a host and close the socket, this will crash the main program.

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@nytamin nytamin requested a review from a team December 4, 2024 10:14
Copy link
Member

@jstarpl jstarpl left a comment

Choose a reason for hiding this comment

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

I think these changes will make pending requestPromise.response promises never resolved or rejected, which would be a potential memory leak. I think the root of the problem might be that most user-code will await the individual _sendCommand-wrapping methods exposing the business functionality, and not really await the requestResponse.promise that would be rejected here.

Maybe one option would be to add a requestResponse.promise.catch(noOp) into _sendRequest to avoid an unhandled rejection exception, while still allowing client code to do it's own catch, if it wants to - but avoiding having floating promises?

@olzzon
Copy link
Author

olzzon commented Dec 10, 2024

I think these changes will make pending requestPromise.response promises never resolved or rejected, which would be a potential memory leak. I think the root of the problem might be that most user-code will await the individual _sendCommand-wrapping methods exposing the business functionality, and not really await the requestResponse.promise that would be rejected here.

but awaiting the requestResponse.promise has never solved the problem. (although it should)

@olzzon
Copy link
Author

olzzon commented Dec 11, 2024

This PR will be closed, solved by guards around things in Sisyfos.
Issues in the way sofie-emberplus-connection is throwing errors still exists.
If a disconnection error occurs, the EmberClient library itself, specifically in the S101Client disconnection handling is throwing an error before it reaches the host programs event handlers.

But as this can be handled in the main program, this PR will be closed.

@olzzon olzzon closed this Dec 11, 2024
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