GNOME Bugzilla – Bug 739544
tcp: Add test and fix memory leak in tcp elements
Last modified: 2014-11-19 16:29:46 UTC
Created attachment 289856 [details] [review] patch: gst-plugins-base: tests: Add socketintegrationtest There don't seem to be any unit tests for the socket handling elements. As I am about to attempt some refactorings I've added some basic tests which exercise some of the happy-paths in tcpclientsrc, tcpserversrc, tcpserversink and tcpclientsink. This lead to the discovery (via make check-valgrind) of 2 leaks in tcpserversink which I have fixed.
Created attachment 289857 [details] [review] patch: gst-plugins-base: tcpserversink: Don't leak a `GSocket` and a `GInetSocketAddress`
Review of attachment 289857 [details] [review]: ::: gst/tcp/gsttcpserversink.c @@ +182,3 @@ #endif + g_object_unref (client_socket); This seems weird. I would expect the socket reference to be stolen by the base class when adding it... and I didn't see where the base class takes an additional refernece to the socket either. Can you check?
Review of attachment 289856 [details] [review]: Looks mostly good, thanks! ::: tests/check/pipelines/socketintegrationtest.c @@ +79,3 @@ + GST_CLOCK_TIME_NONE); + gst_element_get_state (GST_ELEMENT (st->src_pipeline), NULL, NULL, + GST_CLOCK_TIME_NONE); Downwards state changes should be immediate, don't wait for them to finish @@ +82,3 @@ + + g_object_unref (st->sink_pipeline); + g_object_unref (st->src_pipeline); gst_object_unref()
(In reply to comment #2) > Review of attachment 289857 [details] [review]: > > ::: gst/tcp/gsttcpserversink.c > @@ +182,3 @@ > #endif > > + g_object_unref (client_socket); > > This seems weird. I would expect the socket reference to be stolen by the base > class when adding it... and I didn't see where the base class takes an > additional refernece to the socket either. > > Can you check? Yes, this was tricky to track down in the first place. It is passed to the base-class (gst_multi_handle_sink_add_full[1]) but then passed back again via the new_client vmethod to gst_multi_socket_sink_new_client where it's reffed[2]. "transfer-none" seems to be the intention as gst_multi_handle_sink_add_full checks for duplicates, but doesn't do any unreffing if it find one[3]. I guess this makes sense as GstMultiHandleSink wouldn't know how to ref and unref a handle, even if it wanted to. [1]: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp/gstmultihandlesink.c#n643 [2]: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp/gstmultisocketsink.c [3]: http://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst/tcp/gstmultihandlesink.c#n712
Indeed :) Can you add a comment about that to the code? Otherwise it will just confuse the next person looking at it
Created attachment 290102 [details] [review] patch: gst-plugins-base: tests: Add socketintegrationtest Thanks for the review. I've fixed the two issues and here's a new patch.
Created attachment 290106 [details] [review] patch: gst-plugins-base: tcpserversink: Don't leak a `GSocket` and a `GInetSocketAddress` Same patch as before but with added documentation to GstMultiHandleSink clarifying ownership of the handles.
Thanks for the patches. I renamed the test to "tcp" for consistency as all other pipeline tests specific to a plugin are also directly named after the plugin. Will be harder to check if a test already exists for a plugin otherwise :) commit ffb43c059197ece6f203949dfdedfc2a9f18af92 Author: William Manley <will@williammanley.net> Date: Thu Nov 6 14:14:22 2014 +0000 tcpserversink: Don't leak a `GSocket` and a `GInetSocketAddress` when accepting a connection. Discovered by `make check-valgrind` with the new `socketintegrationtest`. https://bugzilla.gnome.org/show_bug.cgi?id=739544 commit 5b0ec93e99a091c9708df83a0f645593d3b31da8 Author: William Manley <will@williammanley.net> Date: Mon Nov 3 01:08:27 2014 +0000 tests: Add TCP pipelines test There don't seem to be any unit tests for the socket handling elements. As I am about to attempt some refactorings I've added some basic tests which exercise some of the happy-paths in tcpclientsrc, tcpserversrc, tcpserversink and tcpclientsink. They should let me know if I've caused serious breakage. They are far from exhaustive but are sufficient for me to have caught a few memory-leaks in the existing code. https://bugzilla.gnome.org/show_bug.cgi?id=739544