From 2958e723fc996e2dd7edfdc6ac53dcdf48323549 Mon Sep 17 00:00:00 2001 From: Joe Shaw Date: Wed, 9 Mar 2005 04:36:15 +0000 Subject: 2005-03-08 Joe Shaw Fix a bunch of lifecycle and memory management problems in the mono bindings. * mono/Arguments.cs (Arguments): Implement IDisposable * mono/Bus.cs (Bus): Don't allow public instantiation. This is strictly a static class. * mono/Connection.cs: Move the DBusObjectPathVTable and associated delegates into this file. (Connection): Implement IDisposable. (Dispose): Disconnect the connection and set the raw connection pointer to IntPtr.Zero. (~Connection): Call Dispose(). (RegisterObjectPath): Added. Manages the registration of object paths so we can cleanly disconnect them at dispose/finalize time. (UnregisterObjectPath): Ditto. (set_RawConnection): Unregister all of the object paths when changing the underlying DBusConnection. Add them back onto the new connection, if any. * mono/Handler.cs: Don't implement IDisposable; it doesn't use any more unmanaged resources anymore, so it's not necessary. Move all the DBusObjectPathVTable stuff out of here. (Handler): Save references to our delegates so that they don't get finalized. Call Connection.RegisterObjectPath() instead of dbus_connection_register_object_path() directly. (Message_Called): Dispose the message after we're finished with it. * mono/Message.cs (Message): Implement IDisposable. (Dispose): Dispose the Arguments, and set the RawMessage to IntPtr.Zero. (SendWithReplyAndBlock): We own the ref to the reply that comes back from dbus_connection_send_with_reply_and_block() so add a comment about that and unref it after we've constructed a managed MethodReturn class around it. Fixes a big, big leak. * mono/ProxyBuilder.cs: Reflect into Message to get the Dispose method. (BuildSignalHandler): After we've sent the Signal message, dispose of it. (BuildMethod): Dispose of the method call and reply messages after we've sent the message and extracted the data we want from the reply. * mono/Service.cs (UnregisterObject): Don't call handler.Dispose() anymore. (Service_FilterCalled): Dispose of the message after we're finished with it. --- ChangeLog | 55 ++++++++++++++++++++++++++- mono/Arguments.cs | 22 +++++++---- mono/Bus.cs | 3 ++ mono/Connection.cs | 100 +++++++++++++++++++++++++++++++++++++++++++------- mono/Handler.cs | 102 +++++++++------------------------------------------ mono/Message.cs | 26 ++++++++++++- mono/ProxyBuilder.cs | 16 +++++++- mono/Service.cs | 4 +- 8 files changed, 217 insertions(+), 111 deletions(-) diff --git a/ChangeLog b/ChangeLog index 91153314..844b73ca 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,58 @@ 2005-03-08 Joe Shaw - + + Fix a bunch of lifecycle and memory management problems + in the mono bindings. + + * mono/Arguments.cs (Arguments): Implement IDisposable + + * mono/Bus.cs (Bus): Don't allow public instantiation. This is + strictly a static class. + + * mono/Connection.cs: Move the DBusObjectPathVTable and associated + delegates into this file. + (Connection): Implement IDisposable. + (Dispose): Disconnect the connection and set the raw connection + pointer to IntPtr.Zero. + (~Connection): Call Dispose(). + (RegisterObjectPath): Added. Manages the registration of object + paths so we can cleanly disconnect them at dispose/finalize time. + (UnregisterObjectPath): Ditto. + (set_RawConnection): Unregister all of the object paths when + changing the underlying DBusConnection. Add them back onto the + new connection, if any. + + * mono/Handler.cs: Don't implement IDisposable; it doesn't use any + more unmanaged resources anymore, so it's not necessary. Move all + the DBusObjectPathVTable stuff out of here. + (Handler): Save references to our delegates so that they don't get + finalized. Call Connection.RegisterObjectPath() instead of + dbus_connection_register_object_path() directly. + (Message_Called): Dispose the message after we're finished with + it. + + * mono/Message.cs (Message): Implement IDisposable. + (Dispose): Dispose the Arguments, and set the RawMessage to + IntPtr.Zero. + (SendWithReplyAndBlock): We own the ref to the reply that comes + back from dbus_connection_send_with_reply_and_block() so add a + comment about that and unref it after we've constructed a managed + MethodReturn class around it. Fixes a big, big leak. + + * mono/ProxyBuilder.cs: Reflect into Message to get the Dispose + method. + (BuildSignalHandler): After we've sent the Signal message, dispose + of it. + (BuildMethod): Dispose of the method call and reply messages after + we've sent the message and extracted the data we want from the + reply. + + * mono/Service.cs (UnregisterObject): Don't call handler.Dispose() + anymore. + (Service_FilterCalled): Dispose of the message after we're + finished with it. + +2005-03-08 Joe Shaw + * dbus/dbus-connection.c (dbus_connection_send_with_reply): After we attach our pending call to the connection, unref it. Fixes a leak. diff --git a/mono/Arguments.cs b/mono/Arguments.cs index 41e6d15d..61ae443f 100644 --- a/mono/Arguments.cs +++ b/mono/Arguments.cs @@ -7,29 +7,37 @@ namespace DBus { // Holds the arguments of a message. Provides methods for appending // arguments and to assist in matching .NET types with D-BUS types. - public class Arguments : IEnumerable + public class Arguments : IEnumerable, IDisposable { // Must follow sizeof(DBusMessageIter) internal const int DBusMessageIterSize = 14*4; private static Hashtable dbusTypes = null; private Message message; - private IntPtr appenderIter = Marshal.AllocCoTaskMem(DBusMessageIterSize); + private IntPtr appenderIter; private IEnumerator enumerator = null; - internal Arguments() + internal Arguments (Message message) { + this.appenderIter = Marshal.AllocCoTaskMem(DBusMessageIterSize); + this.message = message; } - ~Arguments() + private void Dispose (bool disposing) { Marshal.FreeCoTaskMem(appenderIter); } - internal Arguments(Message message) + public void Dispose () { - this.message = message; + Dispose (true); + GC.SuppressFinalize (this); } - + + ~Arguments() + { + Dispose (false); + } + // Checks the suitability of a D-BUS type for supporting a .NET // type. public static bool Suits(Type dbusType, Type type) diff --git a/mono/Bus.cs b/mono/Bus.cs index 963e8195..05cf24c2 100644 --- a/mono/Bus.cs +++ b/mono/Bus.cs @@ -14,6 +14,9 @@ namespace DBus Activation = 2 } + // Don't allow instantiation + private Bus () { } + public static Connection GetSessionBus() { return GetBus(BusType.Session); diff --git a/mono/Connection.cs b/mono/Connection.cs index 6abd7e80..af0764db 100644 --- a/mono/Connection.cs +++ b/mono/Connection.cs @@ -12,7 +12,36 @@ namespace DBus IntPtr rawMessage, IntPtr userData); - public class Connection + internal delegate void DBusObjectPathUnregisterFunction(IntPtr rawConnection, + IntPtr userData); + + internal delegate int DBusObjectPathMessageFunction(IntPtr rawConnection, + IntPtr rawMessage, + IntPtr userData); + + [StructLayout (LayoutKind.Sequential)] + internal struct DBusObjectPathVTable + { + public DBusObjectPathUnregisterFunction unregisterFunction; + public DBusObjectPathMessageFunction messageFunction; + public IntPtr padding1; + public IntPtr padding2; + public IntPtr padding3; + public IntPtr padding4; + + public DBusObjectPathVTable(DBusObjectPathUnregisterFunction unregisterFunction, + DBusObjectPathMessageFunction messageFunction) + { + this.unregisterFunction = unregisterFunction; + this.messageFunction = messageFunction; + this.padding1 = IntPtr.Zero; + this.padding2 = IntPtr.Zero; + this.padding3 = IntPtr.Zero; + this.padding4 = IntPtr.Zero; + } + } + + public class Connection : IDisposable { /// /// A pointer to the underlying Connection structure @@ -26,8 +55,9 @@ namespace DBus private int timeout = -1; - private ArrayList filters = new ArrayList (); // of DBusHandleMessageFunction - private ArrayList matches = new ArrayList (); // of string + private ArrayList filters = new ArrayList (); // of DBusHandleMessageFunction + private ArrayList matches = new ArrayList (); // of string + private Hashtable object_paths = new Hashtable (); // key: string value: DBusObjectPathVTable internal Connection(IntPtr rawConnection) { @@ -49,6 +79,22 @@ namespace DBus SetupWithMain(); } + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + public void Dispose (bool disposing) + { + if (disposing && RawConnection != IntPtr.Zero) + { + dbus_connection_disconnect(rawConnection); + + RawConnection = IntPtr.Zero; // free the native object + } + } + public void Flush() { dbus_connection_flush(RawConnection); @@ -61,17 +107,7 @@ namespace DBus ~Connection () { - if (RawConnection != IntPtr.Zero) - { - foreach (DBusHandleMessageFunction func in this.filters) - RemoveFilter (func); - - foreach (string match_rule in this.matches) - RemoveMatch (match_rule); - - dbus_connection_disconnect(rawConnection); - } - RawConnection = IntPtr.Zero; // free the native object + Dispose (false); } internal static Connection Wrap(IntPtr rawConnection) @@ -121,6 +157,22 @@ namespace DBus this.matches.Remove (match_rule); } + internal void RegisterObjectPath (string path, DBusObjectPathVTable vtable) + { + if (!dbus_connection_register_object_path (RawConnection, path, ref vtable, IntPtr.Zero)) + throw new OutOfMemoryException (); + + this.object_paths[path] = vtable; + } + + internal void UnregisterObjectPath (string path) + { + dbus_connection_unregister_object_path (RawConnection, path); + + this.object_paths.Remove (path); + } + + public string UniqueName { get @@ -178,6 +230,9 @@ namespace DBus foreach (string match_rule in this.matches) dbus_bus_remove_match (rawConnection, match_rule, IntPtr.Zero); + foreach (string path in this.object_paths.Keys) + dbus_connection_unregister_object_path (rawConnection, path); + // Get the reference to this IntPtr rawThis = dbus_connection_get_data (rawConnection, Slot); Debug.Assert (rawThis != IntPtr.Zero); @@ -211,11 +266,17 @@ namespace DBus foreach (string match_rule in this.matches) dbus_bus_add_match (rawConnection, match_rule, IntPtr.Zero); + + foreach (string path in this.object_paths.Keys) { + DBusObjectPathVTable vtable = (DBusObjectPathVTable) this.object_paths[path]; + dbus_connection_register_object_path (rawConnection, path, ref vtable, IntPtr.Zero); + } } else { this.filters.Clear (); this.matches.Clear (); + this.object_paths.Clear (); } } } @@ -278,5 +339,16 @@ namespace DBus private extern static void dbus_bus_remove_match(IntPtr rawConnection, string rule, IntPtr erro); + + [DllImport ("dbus-1")] + private extern static bool dbus_connection_register_object_path (IntPtr rawConnection, + string path, + ref DBusObjectPathVTable vTable, + IntPtr userData); + + [DllImport ("dbus-1")] + private extern static void dbus_connection_unregister_object_path (IntPtr rawConnection, + string path); + } } diff --git a/mono/Handler.cs b/mono/Handler.cs index 9696a4d3..87092f90 100644 --- a/mono/Handler.cs +++ b/mono/Handler.cs @@ -13,7 +13,7 @@ namespace DBus NeedMemory = 2 } - internal class Handler : IDisposable + internal class Handler { private string path = null; private Introspector introspector = null; @@ -21,70 +21,12 @@ namespace DBus private DBusObjectPathVTable vTable; private Connection connection; private Service service; - private bool disposed = false; - - internal delegate void DBusObjectPathUnregisterFunction(IntPtr rawConnection, - IntPtr userData); - - internal delegate int DBusObjectPathMessageFunction(IntPtr rawConnection, - IntPtr rawMessage, - IntPtr userData); - - [StructLayout (LayoutKind.Sequential)] - private struct DBusObjectPathVTable - { - public DBusObjectPathUnregisterFunction unregisterFunction; - public DBusObjectPathMessageFunction messageFunction; - public IntPtr padding1; - public IntPtr padding2; - public IntPtr padding3; - public IntPtr padding4; - - public DBusObjectPathVTable(DBusObjectPathUnregisterFunction unregisterFunction, - DBusObjectPathMessageFunction messageFunction) - { - this.unregisterFunction = unregisterFunction; - this.messageFunction = messageFunction; - this.padding1 = IntPtr.Zero; - this.padding2 = IntPtr.Zero; - this.padding3 = IntPtr.Zero; - this.padding4 = IntPtr.Zero; - } - } - - public void Dispose() - { - Dispose(true); - GC.SuppressFinalize(this); - } - - private void Dispose(bool disposing) - { - if (!disposed) { - if (disposing) { - // Clean up managed resources - } - - service = null; - // Clean up unmanaged resources - if (Connection != null && Connection.RawConnection != IntPtr.Zero && Path != null) { - dbus_connection_unregister_object_path(Connection.RawConnection, - Path); - } - - connection = null; - introspector = null; - handledObject = null; - } - - disposed = true; - } - - ~Handler() - { - Dispose(false); - } + // We need to hold extra references to these callbacks so that they don't + // get garbage collected before they are called back into from unmanaged + // code. + private DBusObjectPathUnregisterFunction unregister_func; + private DBusObjectPathMessageFunction message_func; public Handler(object handledObject, string path, @@ -96,15 +38,11 @@ namespace DBus this.path = path; // Create the vTable and register the path - vTable = new DBusObjectPathVTable(new DBusObjectPathUnregisterFunction(Unregister_Called), - new DBusObjectPathMessageFunction(Message_Called)); - - if (!dbus_connection_register_object_path(Connection.RawConnection, - Path, - ref vTable, - IntPtr.Zero)) - throw new OutOfMemoryException(); + this.unregister_func = new DBusObjectPathUnregisterFunction (Unregister_Called); + this.message_func = new DBusObjectPathMessageFunction (Message_Called); + vTable = new DBusObjectPathVTable (this.unregister_func, this.message_func); + Connection.RegisterObjectPath (Path, vTable); RegisterSignalHandlers(); } @@ -131,8 +69,6 @@ namespace DBus set { this.handledObject = value; - object[] attributes; - // Register the methods this.introspector = Introspector.GetIntrospector(value.GetType()); } @@ -153,17 +89,22 @@ namespace DBus IntPtr userData) { Message message = Message.Wrap(rawMessage, Service); + Result res = Result.NotYetHandled; switch (message.Type) { + case Message.MessageType.MethodCall: + res = HandleMethod ((MethodCall) message); + break; + case Message.MessageType.Signal: // We're not interested in signals here because we're the ones // that generate them! break; - case Message.MessageType.MethodCall: - return (int) HandleMethod((MethodCall) message); } - return (int) Result.NotYetHandled; + message.Dispose (); + + return (int) res; } private Result HandleMethod(MethodCall methodCall) @@ -227,12 +168,5 @@ namespace DBus this.service = value; } } - - [DllImport ("dbus-1")] - private extern static bool dbus_connection_register_object_path (IntPtr rawConnection, string path, ref DBusObjectPathVTable vTable, IntPtr userData); - - [DllImport ("dbus-1")] - private extern static void dbus_connection_unregister_object_path (IntPtr rawConnection, string path); - } } diff --git a/mono/Message.cs b/mono/Message.cs index 5aa3542f..944e3f92 100644 --- a/mono/Message.cs +++ b/mono/Message.cs @@ -6,7 +6,7 @@ namespace DBus using System.Diagnostics; using System.Collections; - public class Message + public class Message : IDisposable { private static Stack stack = new Stack (); @@ -83,10 +83,26 @@ namespace DBus { this.service = service; } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } - ~Message() + public void Dispose (bool disposing) { + if (disposing) { + if (this.arguments != null) + this.arguments.Dispose (); + } + RawMessage = IntPtr.Zero; // free the native object + } + + ~Message() + { + Dispose (false); } public static Message Wrap(IntPtr rawMessage, Service service) @@ -198,6 +214,12 @@ namespace DBus if (rawMessage != IntPtr.Zero) { MethodReturn methodReturn = new MethodReturn(rawMessage, Service); + // Ownership of a ref is passed onto us from + // dbus_connection_send_with_reply_and_block(). It gets reffed as + // a result of being passed into the MethodReturn ctor, so unref + // the extra one here. + dbus_message_unref (rawMessage); + return methodReturn; } else { throw new DBusException(error); diff --git a/mono/ProxyBuilder.cs b/mono/ProxyBuilder.cs index 80449093..fefac473 100644 --- a/mono/ProxyBuilder.cs +++ b/mono/ProxyBuilder.cs @@ -35,6 +35,8 @@ namespace DBus new Type[0]); private static MethodInfo Message_SendMI = typeof(Message).GetMethod("Send", new Type[0]); + private static MethodInfo Message_DisposeMI = typeof(Message).GetMethod("Dispose", + new Type[0]); private static MethodInfo Arguments_GetEnumeratorMI = typeof(Arguments).GetMethod("GetEnumerator", new Type[0]); private static MethodInfo IEnumerator_MoveNextMI = typeof(System.Collections.IEnumerator).GetMethod("MoveNext", @@ -197,7 +199,6 @@ namespace DBus // Generate the locals LocalBuilder methodCallL = generator.DeclareLocal(typeof(MethodCall)); methodCallL.SetLocalSymInfo("signal"); - LocalBuilder replyL = generator.DeclareLocal(typeof(MethodReturn)); //generator.EmitWriteLine("Signal signal = new Signal(...)"); generator.Emit(OpCodes.Ldsfld, serviceF); @@ -224,6 +225,10 @@ namespace DBus generator.Emit(OpCodes.Ldloc_0); generator.EmitCall(OpCodes.Callvirt, Message_SendMI, null); + //generator.EmitWriteLine("signal.Dispose()"); + generator.Emit(OpCodes.Ldloc_0); + generator.EmitCall(OpCodes.Callvirt, Message_DisposeMI, null); + //generator.EmitWriteLine("return"); generator.Emit(OpCodes.Ret); } @@ -310,6 +315,15 @@ namespace DBus } } + // Clean up after ourselves + //generator.EmitWriteLine("methodCall.Dispose()"); + generator.Emit(OpCodes.Ldloc_0); + generator.EmitCall(OpCodes.Callvirt, Message_DisposeMI, null); + + //generator.EmitWriteLine("reply.Dispose()"); + generator.Emit(OpCodes.Ldloc_1); + generator.EmitCall(OpCodes.Callvirt, Message_DisposeMI, null); + if (method.ReturnType != typeof(void)) { generator.Emit(OpCodes.Ldloc_3); } diff --git a/mono/Service.cs b/mono/Service.cs index 4280c6b3..40703a53 100644 --- a/mono/Service.cs +++ b/mono/Service.cs @@ -75,9 +75,7 @@ namespace DBus public void UnregisterObject(object handledObject) { - Handler handler = (Handler) registeredHandlers[handledObject]; registeredHandlers.Remove(handledObject); - handler.Dispose(); } public void RegisterObject(object handledObject, @@ -128,6 +126,8 @@ namespace DBus } } + message.Dispose (); + return (int) Result.NotYetHandled; } -- cgit