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

PR - Bugfix for graceful closing when MaxConcurrentCalls exceeds 1. #21

Merged
merged 7 commits into from
Apr 23, 2019

Conversation

Tankatronic
Copy link
Contributor

No description provided.

@Tankatronic
Copy link
Contributor Author

In reference to #20

- Use MaxConcurrentCalls property instead of ConcurrencyCount
- Fix typo's
@loekd
Copy link
Owner

loekd commented Apr 18, 2019

Thank you for this contribution @Tankatronic !
I've added some changes, please let me know how you feel about those.

@Tankatronic
Copy link
Contributor Author

I like the changes and agree with that refactor for ListenForMessages. I didn't like the idea of having to put the initialization logic for the Semaphores in the derived class as that passed the burden on any additional implementations, but I didn't want to go too crazy! Looks good!

@loekd
Copy link
Owner

loekd commented Apr 19, 2019

Yes, you're right. I've made some (potentially breaking) changes to move initialization to where it belongs.
Let me know if that's better, and I'll merge it if true.

@loekd loekd merged commit 119cbbe into loekd:master Apr 23, 2019
@Tankatronic
Copy link
Contributor Author

Sorry for the delay, I saw the message but didn't have time to look and forgot later! The changes looked good to me and it feels better having that logic living in the base class now.

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