-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add note about freeing transaction in handle_message #3374
Add note about freeing transaction in handle_message #3374
Conversation
This is incorrect. You only need to free |
You're right, it's the one from handle_message I'm freeing now. I put the comment in the wrong place then, I'll update the PR. |
dbb3135
to
4bccd18
Compare
I moved the comment at the right place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added some notes.
…face documentation
4bccd18
to
d6fccd8
Compare
I made the changes. I also added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I added just a couple of minor notes after re-reading the changes, but after those I think this is good to be merged.
src/plugins/plugin.h
Outdated
@@ -283,6 +283,10 @@ struct janus_plugin { | |||
* @param[out] error An integer that may contain information about any error */ | |||
void (* const create_session)(janus_plugin_session *handle, int *error); | |||
/*! \brief Method to handle an incoming message/request from a peer | |||
* @note The Janus core increases the references to both the \c message and \c jsep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, since in this case this is a method that goes from core to plugin, it's more correct to say something like "The Janus core leaves ownership of both ...... objects to plugins. Ths means..". The "increases" part makes more sense for push_event
since in that case the core does indeed increase the references to those objects, since plugins may decrease their own.
src/plugins/plugin.h
Outdated
* @note The Janus core increases the references to both the \c message and \c jsep | ||
* json_t objects. This means that you'll have to decrease your own | ||
* reference yourself with a \c json_decref when you're done with them. | ||
* You'll also have to free \c transaction with \c g_free. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better remove the dot at the end, after g_free
, since otherwise it will appear as part of the code
block. Besides, we don't have it at the end of lines in most of the other doxygen comments.
src/plugins/plugin.h
Outdated
* json_t objects. This means that you'll have to decrease your own | ||
* reference yourself with a \c json_decref after calling push_event. | ||
* reference yourself with a \c json_decref after calling \c push_event. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (it was there before, but now there's \c
)
That's valid feedback. I did the changes. |
Thanks! Merging 👍 |
Add note about freeing transaction after calling
push_event
in plugin interface documentation.The only place this was mentioned seems to be in this comment in the
janus.c
janus-gateway/src/janus.c
Line 1759 in 9be6a74
and in plugin implementation like
janus-gateway/src/plugins/janus_textroom.c
Lines 1261 to 1265 in 9be6a74
We had a memory leak for this in the rust sfu plugin I fixed in
networked-aframe/janus-plugin-sfu@4226d00
that only showed in ASan output when specifying loop_events option.
The original creator of the plugin probably overlooked this because this was not documented in
plugin.h
.