Client send heartbeat periodically

[comzyh]

try to fix #96

I will remove logger.debug if you think it's not necessary.

[mpaolini]

Just had a quick look. Maybe you should cancel the heartbeat worker on shutdown?

[comzyh]

@mpaolini

You are right. I use while loop because I can't find a method like set_timeout or whatever.

But maybe the condition while self.is_open is enough ?

https://github.com/Polyconseil/aioamqp/pull/97/files#diff-a1512baaedd3b5456a3a9eea30fc6297R314

I make this patch reference to https://github.com/pika/pika/blob/502aa0e6fdb57274aa1583138081eefc6b6e8f62/pika/heartbeat.py#L159

[mpaolini]

tested in staging here, seems to be working OK.

I made a small change here in this branch of mine https://github.com/Polyconseil/aioamqp/compare/master...elastic-coders:heartbeat_fix#diff-a1512baaedd3b5456a3a9eea30fc6297R315

Feel free to use or drop my own change

[comzyh]

I'm not good at English, lol. So, does "tested in staging" means "tested in production environment" ?

And. I can't fully understand the diff your give above, there a part of my code in that diff. I can see your modification in _run_heartbeat_timer function.

And I think your patch may bring a bug here(maybe):

I also add small enhancement to log a warning if the server does not reply to our own hearbeats

pay attention to this line https://github.com/elastic-coders/aioamqp/commit/53a6c89a04aa02c7c82319ace78d9fdf0570c8ea?diff=unified#diff-a1512baaedd3b5456a3a9eea30fc6297R315

Based on my understand of RabbitMQ heartbeat protocol, Server and Client just have to send a heartbeat before _last_heartbeattime + timeout.

And. Server don't have to send a heartbeat as a reply of heartbeat from client immediately. (IMPORTANT 1)

In this line, you set timeout=self.server_heartbeat / 2, so a server working correctly my not send a heartbeat in timeout / 2 seconds.

so your code may raise false-alarm

Then your code may bring in another bug.

Based on IMPORTANT 1, heartbeat function may finish(this is a coroutine. i cant find better word, maybe 'return'?) after a few seconds(let it be t1), so the interval of heartbeat may be increased by t1, i think that's not right.

I'm looking forward to your reply.

[mpaolini]

@comzyh

So, does "tested in staging" means "tested in production environment" ?

It means that is running on our servers but only for developers and internal testers to enjoy

Based on IMPORTANT 1, heartbeat function may finish(this is a coroutine. i cant find better word, maybe 'return'?) after a few seconds(let it be t1), so the interval of heartbeat may be increased by t1, i think that's not right.

rightly so, my patch can definitely delay the client heartbeat.

Keep in mind that hearbeat() does return everytime a message is received from server. Not just when a heartbeat is received.

In my patch I just tried to point out that we might need some modification to the current ._heartbeat_waiter and .heartbeat() functions. But your patch is valid even without those.

please @dzen merge this ;)

[mpaolini]

@comzyh as a matter of facts, I do see sporadic warnings in staging using my own patch, so it is not good

[RemiCardona]

I'm not exactly thrilled about the patches here, though I agree they do improve the poor state of the heartbeat feature as a whole.

I'll try to come up with a patch soon (today or tomorrow) and I'll run it by you guys. @mpaolini I'd be grateful if you could give it a spin as well.

Thanks to both of you for the patches and your patience, it's greatly appreciated.

[comzyh]

Keep in mind that hearbeat() does return everytime a message is received from server. Not just when a heartbeat is received.

@mpaolini I do forget this, thank you.

[RemiCardona]

PR #96 was merged, closing this one. Thanks again for the tests and your patience.

Cheers

[comzyh]

Cheers