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. |
vincentfretin
force-pushed
the
add-doc-free-transaction
branch
from
dbb3135
to
4bccd18
Compare
vincentfretin
changed the title
|
I moved the comment at the right place. |
…face documentation
vincentfretin
force-pushed
the
add-doc-free-transaction
branch
from
4bccd18
to
d6fccd8
Compare
|
I made the changes. I also added the |
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.
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.
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.
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_eventin plugin interface documentation.The only place this was mentioned seems to be in this comment in the
janus.cjanus-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.