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

Added SetHeartbeatHandler to Session interface #25

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

imkira
Copy link

@imkira imkira commented May 8, 2015

I need this functionality in order to avoid duplicating the logic of handling heartbeats in my own code.
I could do it, but there would be a waste of timers going around.

I have a doubt about the heartbeat function.
I did change it so that a handler can call say session.Send or even session.SetHeartbeatHandler(nil) anything else even if that requires locking. If i call it inside the mutex, that won't be possible.
The most obvious "drawback" of this, is that if the timer is "too fast" (like in the order of a few nanoseconds, which shouldn't happen in principle) the handler may be called concurrently, or if the handler itself does something like session.SetHeartbeatHandler(newHandler) an old handler could be called, because it is called outside the mutex. So, some care should be taken regarding that.

I can also add tests, but want to know first what you think of it.

What do you think of this?


This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.23%) to 93.2% when pulling 168f01f on imkira:feature/addHeartbeatHandler2 into 6571859 on igm:v2.

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

Successfully merging this pull request may close these issues.

3 participants