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

awaitable send #46

Closed
wants to merge 1 commit into from
Closed

awaitable send #46

wants to merge 1 commit into from

Conversation

amarzavery
Copy link
Collaborator

#45

@amarzavery
Copy link
Collaborator Author

@swilliams-a3digital - Do take a look at this PR. However, I am not sure if this library is the right place for this implementation.

  • The method could have a retry logic, which I have not implemented here.
  • Another problem is that event listeners will be added and removed for every send. node.js allows 11 listeners to be attached on an event emitter object by default. After that it emits memory leak warning. Although this limit can be increased by setting an environment variable.

A better implementation would be to add event listeners once and then maintain a map of Promises that can be resolved or rejected based on the event that was fired. In this way one does not hit the default event listener limit of 11 and things move faster.

@amarzavery
Copy link
Collaborator Author

closing in favor of #44.

@amarzavery amarzavery closed this Jun 15, 2019
@swilliams-a3digital
Copy link
Contributor

I agree that one listener with a map of promises is probably necessary.

@amarzavery
Copy link
Collaborator Author

@swilliams-a3digital - Please take a look at the AsynchronousSender that implements the send operation using an internal map.

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