[Asterisk-code-review] res/res_stasis: Fixed wrong StasisEnd timestamp (...asterisk[13])

[sungtae kim]

sungtae kim has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/11133 )

Change subject: res/res_stasis: Fixed wrong StasisEnd timestamp
......................................................................


Patch Set 1:

(2 comments)

> Patch Set 1:
> 
> (2 comments)
> 
> StasisStart gets its timestamp from the start_message_blob, which just obtains it from the current system time and passes it in. Which should still make it unique/different from other events (as I don't see any other event referencing the start_message_blob).

Hi Kevin,

Thank you for your kind reviewing. :)

I've fixed it as your comment and replied it.

About the why I think this is bug is,

I think, the StasisStart/StasisEnd is must be placed into the channel's life circle. Especially between 'ChannelCreated' and 'ChannelDestroyed' ARI event.

Because it's Channel event, it doesn't make sense receiving channel event after ChannelDestroyed event. So I'm trying to fixing it.

StasisStart creates timestamp when the message was creating.
StasisEnd was creating timestamp when the message was sending.

ChannelDestroyed creates timestamp when the event(channel) was occurred(it gets the timestamp from channel's blob).

It's very small different, but in the messages, sometimes, StasisEnd's timestamp has later than ChannelDestroyed. And StasisEnd sent it faster then ChannelDestroyed. Which is doesn't make sense.

https://gerrit.asterisk.org/#/c/11133/1/res/res_stasis.c 
File res/res_stasis.c:

https://gerrit.asterisk.org/#/c/11133/1/res/res_stasis.c@128 
PS1, Line 128: 	msg = ast_json_pack("{s: s, s: O, s: o}",
> Bumping the ref here makes sense to me, but (as Josh pointed out) this is now doing something simila […]
Ah, I've got it. Fixed it. :)

Btw, this branch is 13. I was working in the master to make patch. In the master branch it dump the ref like this for StasisStart.

But I agree with your point. :) It might be causing some unexpected ref free. I fixed it.


https://gerrit.asterisk.org/#/c/11133/1/res/res_stasis.c@130 
PS1, Line 130: 		"timestamp", ast_json_object_get(payload->blob, "timestamp"),
> The blob here gets created in "app_send_end_msg" correct? If so I am not seeing where the timestamp  […]
You're right, and it does creating the timestamp there.

app_send_end_msg() -> ast_channel_blob_create() -> create_channel_blob_message() -> stasis_message_create() -> stasis_message_create_full() -> 'message->timestamp = ast_tvnow();'



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/11133
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: I5eb8380fc472f1100535a6bc4983c64767026116
Gerrit-Change-Number: 11133
Gerrit-PatchSet: 1
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 21:42:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190314/66d8663d/attachment-0001.html>