http://bugzilla.zeroc.com/bugzilla/show_bug.cgi?id=1391. Fixed GC bug with

    communicator.

9 files changed, 337 insertions, 99 deletions

diff --git a/rb/CHANGES b/rb/CHANGES
index 2abc98f532b..410121eba91 100644
--- a/rb/CHANGES
+++ b/rb/CHANGES
@@ -3,6 +3,16 @@ NOTE: Please keep changes in the appropriate section for HEAD or 3.1.
 Changes since version 3.1.1 (HEAD)
 ---------------------------     
 
+- Added support for a user callback when SIGINT like signals are
+  sent to applications that use the Application interface. See
+  Application::interruptCallback and Application::callbackOnInterrupt
+  for details. Fixed support for holdInterrupt and releaseInterrupt.
+
+- Fixed bug with Application.holdInterrupt.
+
+- Fixed a bug that would cause the plug-in to abort if a GC occurred
+  after the communicator was destroyed.
+
 - Fixed a code-generation bug that occurred when the name of a
   data member begins with an uppercase letter.
 
diff --git a/rb/config/Make.rules b/rb/config/Make.rules
index f964b21d031..1b7b1d1c3c5 100644
--- a/rb/config/Make.rules
+++ b/rb/config/Make.rules
@@ -45,13 +45,6 @@ else
     RUBY = ruby
 endif
 
-RUBY_VERSION		= $(shell $(RUBY) --version | cut -d' ' -f2)
-ifneq ($(RUBY_VERSION),1.8.4)
-    ifneq ($(RUBY_VERSION),1.8.5)
-	$(error "Ruby 1.8.4 or 1.8.5 is required - found $(RUBY_VERSION)")
-    endif
-endif
-
 RUBY_INCLUDE_DIR	= $(shell $(RUBY) -e 'require "rbconfig"; puts Config::expand("$$(archdir)")')
 RUBY_LIB_DIR		= $(shell $(RUBY) -e 'require "rbconfig"; puts Config::expand("$$(libdir)")')
 
@@ -143,16 +136,16 @@ SLICE2RBFLAGS		= $(ICECPPFLAGS)
 LDFLAGS			= $(LDPLATFORMFLAGS) $(CXXFLAGS) -L$(libdir)
 
 #
-# Default functions for shared library names. A Ruby extension library
-# cannot have a "lib" prefix, so Ruby-specific functions are defined.
+# Default functions for shared library names. Note that since ruby
+# extension libraries cannot have a "lib" prefix lib is left out.
 #
 
 ifeq ($(mklibfilename),)
-    mklibfilename	= $(if $(2),lib$(1).so.$(2),lib$(1).so)
+    mklibfilename	= $(if $(2),lib$(1).so.$(2),$(1).so)
 endif
 
 ifeq ($(mksoname),)
-    mksoname		= $(if $(2),lib$(1).so.$(2),lib$(1).so)
+    mksoname		= $(if $(2),lib$(1).so.$(2),$(1).so)
 endif
 
 ifeq ($(mklibname),)
@@ -194,14 +187,6 @@ ifeq ($(mkdir),)
 			  chmod a+rx $(1)
 endif
 
-#
-# A Ruby extension library cannot have a "lib" prefix, so Ruby-specific
-# functions are defined that strip "lib" from the regular library name.
-#
-mkrubylibfilename	= $(subst lib,,$(call mklibfilename,$(1),$(2)))
-mkrubysoname		= $(subst lib,,$(call mksoname,$(1),$(2)))
-mkrubylibname		= $(subst lib,,$(call mklibname,$(1)))
-
 SLICE2RB		= $(ICE_HOME)/bin/slice2rb
 
 EVERYTHING		= all depend clean install
diff --git a/rb/demo/Ice/hello/Client.rb b/rb/demo/Ice/hello/Client.rb
index 2e913f57581..2c676ffb9e2 100644
--- a/rb/demo/Ice/hello/Client.rb
+++ b/rb/demo/Ice/hello/Client.rb
@@ -31,7 +31,22 @@ MENU
 end
 
 class Client < Ice::Application
+    def interruptCallback(sig)
+        begin
+            Ice::Application::communicator.destroy
+        rescue => ex
+            puts ex
+        end
+        exit(0)
+    end
+
     def run(args)
+        #
+        # Since this is an interactive demo we want the custom interrupt
+        # callback to be called when the process is interrupted.
+        #
+        Ice::Application::callbackOnInterrupt
+
 	twoway = Demo::HelloPrx::checkedCast(
 	    Ice::Application::communicator().propertyToProxy('Hello.Proxy').
 	        ice_twoway().ice_timeout(-1).ice_secure(false))
diff --git a/rb/demo/Ice/session/Client.rb b/rb/demo/Ice/session/Client.rb
index 7c33fa47fab..b62c1fe8fe4 100644
--- a/rb/demo/Ice/session/Client.rb
+++ b/rb/demo/Ice/session/Client.rb
@@ -54,7 +54,31 @@ class SessionRefreshThread
 end
 
 class Client < Ice::Application
+
+    def initialize
+        @mutex = Mutex.new
+        @session = nil
+        @refresh = nil
+        @refreshThread = nil
+    end
+
+    def interruptCallback(sig)
+        cleanup(true)
+        begin
+            Ice::Application::communicator.destroy
+        rescue => ex
+            puts ex
+        end
+        exit(0)
+    end
+
     def run(args)
+        #
+        # Since this is an interactive demo we want the custom interrupt
+        # callback to be called when the process is interrupted.
+        #
+        Ice::Application::callbackOnInterrupt
+
 	while true
 	    print "Please enter your name ==> "
 	    STDOUT.flush
@@ -71,10 +95,13 @@ class Client < Ice::Application
 	    return false
 	end
 
-	session = factory.create(name)
+        @mutex.synchronize {
+            @session = factory.create(name)
+	    @refresh = SessionRefreshThread.new(Ice::Application::communicator().getLogger(), 5, @session)
+	    @refreshThread = Thread.new { @refresh.run }
+        }
+
 	begin
-	    refresh = SessionRefreshThread.new(Ice::Application::communicator().getLogger(), 5, session)
-	    refreshThread = Thread.new { refresh.run }
 
 	    hellos = []
 
@@ -97,7 +124,7 @@ class Client < Ice::Application
 				 "Use `c' to create a new hello object."
 			end
 		    elsif c == 'c'
-			hellos.push(session.createHello())
+			hellos.push(@session.createHello())
 			puts "Created hello object " + (hellos.length - 1).to_s
 		    elsif c == 's'
 			destroy = false
@@ -124,13 +151,7 @@ class Client < Ice::Application
 	    # is set to 0 so that if session->destroy() raises an exception
 	    # the thread will not be re-terminated and re-joined.
 	    #
-	    refresh.terminate
-	    refreshThread.join
-	    refresh = nil
-
-	    if destroy
-		session.destroy()
-	    end
+            cleanup(destroy)
 	    if shutdown
 		factory.shutdown()
 	    end
@@ -139,15 +160,27 @@ class Client < Ice::Application
 	    # The refresher thread must be terminated in the event of a
 	    # failure.
 	    #
-	    if refresh
-		refresh.terminate
-		refreshThread.join
-	    end
+            cleanup(true)
 	end
 
 	return true
     end
 
+    def cleanup(destroy)
+        @mutex.synchronize {
+            if @refresh
+                @refresh.terminate
+                @refreshThread.join
+                @refresh = nil
+            end
+
+            if destroy && @session
+                @session.destroy
+                @session = nil
+            end
+        }
+    end
+
     def menu
 	print <<MENU
 usage:
diff --git a/rb/demo/Ice/throughput/Client.rb b/rb/demo/Ice/throughput/Client.rb
index 438d0e7186c..608f75105e3 100644
--- a/rb/demo/Ice/throughput/Client.rb
+++ b/rb/demo/Ice/throughput/Client.rb
@@ -36,7 +36,22 @@ MENU
 end
 
 class Client < Ice::Application
+    def interruptCallback(sig)
+        begin
+            Ice::Application::communicator.destroy
+        rescue => ex
+            puts ex
+        end
+        exit(0)
+    end
+
     def run(args)
+        #
+        # Since this is an interactive demo we want the custom interrupt
+        # callback to be called when the process is interrupted.
+        #
+        Ice::Application::callbackOnInterrupt
+
 	throughput = Demo::ThroughputPrx::checkedCast(
 	    Ice::Application::communicator().propertyToProxy('Throughput.Throughput'))
 	if not throughput
diff --git a/rb/ruby/Ice.rb b/rb/ruby/Ice.rb
index 65eb8074bd7..c947100d456 100644
--- a/rb/ruby/Ice.rb
+++ b/rb/ruby/Ice.rb
@@ -84,6 +84,101 @@ require 'Ice/Router.rb'
 
 module Ice
     #
+    # Note the interface is the same as the C++ CtrlCHandler
+    # implementation, however, the implementation is different.
+    #
+    class CtrlCHandler
+        def initialize
+            if @@_self != nil
+		raise RuntimeError, "Only a single instance of a CtrlCHandler can be instantiated."
+            end
+            @@_self = self
+
+            # State variables. These are not class static variables.
+            @condVar = ConditionVariable.new
+            @mutex = Mutex.new
+            @queue = Array.new
+            @done = false
+            @callback = nil
+
+            #
+            # Setup and install signal handlers
+            #
+	    if Signal.list.has_key?('HUP')
+		Signal.trap('HUP') { signalHandler('HUP') }
+	    end
+	    Signal.trap('INT') { signalHandler('INT') }
+	    Signal.trap('TERM') { signalHandler('TERM') }
+
+            @thr = Thread.new { main }
+        end
+
+        # Dequeue and dispatch signals.
+        def main
+            while true
+                sig, callback = @mutex.synchronize {
+                    while @queue.empty? and not @done
+                        @condVar.wait(@mutex)
+                    end
+                    if @done
+                        return
+                    end
+                    @queue.shift
+                }
+                if callback
+                    callback.call(sig)
+                end
+            end
+        end
+
+        # Destroy the object. Wait for the thread to terminate and cleanup
+        # the internal state.
+        def destroy
+            @mutex.synchronize {
+                @done = true
+                @condVar.signal
+            }
+
+            # Wait for the thread to terminate
+            @thr.join
+
+            #
+            # Cleanup any state set by the CtrlCHandler.
+            #
+	    if Signal.list.has_key?('HUP')
+		Signal.trap('HUP', 'SIG_DFL')
+	    end
+	    Signal.trap('INT', 'SIG_DFL')
+	    Signal.trap('TERM', 'SIG_DFL')
+            @@_self = nil
+        end
+
+        def setCallback(callback)
+            @mutex.synchronize {
+                @callback = callback
+            }
+        end
+
+        def getCallback
+            @mutex.synchronize {
+                return @callback
+            }
+        end
+
+        # Private. Only called by the signal handling mechanism.
+        def signalHandler(sig)
+            @mutex.synchronize {
+                #
+                # The signal AND the current callback are queued together.
+                #
+                @queue = @queue.push([sig, @callback])
+                @condVar.signal
+            }
+        end
+        @@_self = nil
+    end
+
+    #
     # Ice::Application.
     #
     class Application
@@ -98,20 +193,11 @@ module Ice
 		print $0 + ": only one instance of the Application class can be used"
 		return false
 	    end
+            @@_ctrlCHandler = CtrlCHandler.new
 
 	    @@_interrupted = false
 	    @@_appName = $0
 
-	    #
-	    # Install our handler for the signals we are interested in. We assume main()
-	    # is called from the main thread.
-	    #
-	    if Signal.list.has_key?('HUP')
-		Signal.trap('HUP') { Application::signalHandler('HUP') }
-	    end
-	    Signal.trap('INT') { Application::signalHandler('INT') }
-	    Signal.trap('TERM') { Application::signalHandler('TERM') }
-
 	    status = 0
 
 	    begin
@@ -122,10 +208,12 @@ module Ice
 		    initData.properties = Ice::createProperties
 		    initData.properties.load(configFile)
 		end
+		@@_application = self
 		@@_communicator = Ice::initialize(args, initData)
+		@@_destroyed = false
 
 		#
-		# Used by destroyOnInterruptCallback and shutdownOnInterruptCallback.
+		# Used by destroyOnInterruptCallback.
 		#
 		@@_nohup = @@_communicator.getProperties().getPropertyAsInt("Ice.Nohup") > 0
 
@@ -141,12 +229,31 @@ module Ice
 		status = 1
 	    end
 
-	    if @@_communicator
-		#
-		# We don't want to handle signals anymore.
-		#
-		Application::ignoreInterrupt
+            #
+            # Don't want any new interrupt and at this point (post-run),
+            # it would not make sense to release a held signal to run
+            # shutdown or destroy.
+            #
+            Application::ignoreInterrupt
+
+            @@_mutex.synchronize {
+                while @@_callbackInProgress
+                    @@_condVar.wait(@@_mutex)
+                end
+                if @@_destroyed
+                    @@_communicator = nil
+                else
+                    @@_destroyed = true
+                end
+                #
+                # And _communicator != 0, meaning will be destroyed
+                # next, _destroyed = true also ensures that any
+                # remaining callback won't do anything
+                #
+                @@_application = nil
+            }
 
+	    if @@_communicator
 		begin
 		    @@_communicator.destroy()
 		rescue => ex
@@ -158,6 +265,9 @@ module Ice
 		@@_communicator = nil
 	    end
 
+            @@_ctrlCHandler.destroy()
+            @@_ctrlCHandler = nil
+
 	    return status
 	end
 
@@ -165,6 +275,9 @@ module Ice
 	    raise RuntimeError, 'run() not implemented'
 	end
 
+	def interruptCallback(sig)
+	end
+
 	def Application.appName
 	    @@_appName
 	end
@@ -175,46 +288,44 @@ module Ice
 
 	def Application.destroyOnInterrupt
 	    @@_mutex.synchronize {
-		if @@_ctrlCHandler == @@_holdInterruptCallbackProc
+		if @@_ctrlCHandler.getCallback == @@_holdInterruptCallbackProc
 		    @@_released = true
-		    @@_ctrlCHandler = @@_destroyOnInterruptCallbackProc
 		    @@_condVar.signal
-		else
-		    @@_ctrlCHandler = @@_destroyOnInterruptCallbackProc
 		end
+                @@_ctrlCHandler.setCallback(@@_destroyOnInterruptCallbackProc)
 	    }
 	end
 
-	def Application.shutdownOnInterrupt
+        # No support for this since no server side in Ice for ruby.
+	#def Application.shutdownOnInterrupt
+	#end
+
+	def Application.ignoreInterrupt
 	    @@_mutex.synchronize {
-		if @@_ctrlCHandler == @@_holdInterruptCallbackProc
+		if @@_ctrlCHandler.getCallback == @@_holdInterruptCallbackProc
 		    @@_released = true
-		    @@_ctrlCHandler = @@_shutdownOnInterruptCallbackProc
 		    @@_condVar.signal
-		else
-		    @@_ctrlCHandler = @@_shutdownOnInterruptCallbackProc
 		end
+                @@_ctrlCHandler.setCallback(nil)
 	    }
 	end
 
-	def Application.ignoreInterrupt
+        def Application.callbackOnInterrupt()
 	    @@_mutex.synchronize {
-		if @@_ctrlCHandler == @@_holdInterruptCallbackProc
+		if @@_ctrlCHandler.getCallback == @@_holdInterruptCallbackProc
 		    @@_released = true
-		    @@_ctrlCHandler = nil
 		    @@_condVar.signal
-		else
-		    @@_ctrlCHandler = nil
-		end
+                end
+                @@_ctrlCHandler.setCallback(@@_callbackOnInterruptCallbackProc)
 	    }
 	end
 
 	def Application.holdInterrupt
 	    @@_mutex.synchronize {
-		if @@_ctrlCHandler != @@_holdInterruptCallbackProc
-		    @@_previousCallback = @@_ctrlCHandler
+		if @@_ctrlCHandler.getCallback != @@_holdInterruptCallbackProc
+		    @@_previousCallback = @@_ctrlCHandler.getCallback
 		    @@_released = false
-		    @@_ctrlCHandler = @@_holdInterruptCallbackProc
+		    @@_ctrlCHandler.setCallback(@@_holdInterruptCallbackProc)
 		end
 		# else, we were already holding signals
 	    }
@@ -222,7 +333,7 @@ module Ice
 
 	def Application.releaseInterrupt
 	    @@_mutex.synchronize {
-		if @@_ctrlCHandler == @@_holdInterruptCallbackProc
+		if @@_ctrlCHandler.getCallback == @@_holdInterruptCallbackProc
 		    #
 		    # Note that it's very possible no signal is held;
 		    # in this case the callback is just replaced and
@@ -230,7 +341,7 @@ module Ice
 		    # do no harm.
 		    #
 		    @@_released = true
-		    @@_ctrlCHandler = @@_previousCallback
+		    @@_ctrlCHandler.setCallback(@@_previousCallback)
 		    @@_condVar.signal
 		end
 		# Else nothing to release.
@@ -243,34 +354,33 @@ module Ice
 	    }
 	end
 
-	def Application.signalHandler(sig)
-	    if @@_ctrlCHandler
-		@@_ctrlCHandler.call(sig)
-	    end
-	end
-
 	def Application.holdInterruptCallback(sig)
-	    @@_mutex.synchronize {
+	    callback = @@_mutex.synchronize {
 		while not @@_released
 		    @@_condVar.wait(@@_mutex)
 		end
+                if @@_destroyed
+                    return
+                end
+                @@_ctrlCHandler.getCallback
 	    }
 
 	    #
 	    # Use the current callback to process this (old) signal.
 	    #
-	    if @@_ctrlCHandler
-		@@_ctrlCHandler.call(sig)
+	    if callback
+		callback.call(sig)
 	    end
 	end
 
 	def Application.destroyOnInterruptCallback(sig)
-	    if @@_nohup and sig == 'HUP'
-		return
-	    end
-
 	    @@_mutex.synchronize {
+                if @@_destroyed or @@_nohup and sig == 'HUP'
+                    return
+                end
+		@@_callbackInProcess = true
 		@@_interrupted = true
+		@@_destroyed = true
 	    }
 
 	    begin
@@ -280,37 +390,53 @@ module Ice
 		puts @@_appName + " (while destroying in response to signal " + sig + "):"
 		puts ex.backtrace.join("\n")
 	    end
+	    @@_mutex.synchronize {
+		@@_callbackInProcess = false
+		@@_condVar.signal
+	    }
 	end
 
-	def Application.shutdownOnInterruptCallback(sig)
-	    if @@_nohup and sig == 'HUP'
-		return
-	    end
-
+	def Application.callbackOnInterruptCallback(sig)
+            # For SIGHUP the user callback is always called. It can
+            # decide what to do.
 	    @@_mutex.synchronize {
+                if @@_destroyed
+                    #
+                    # Being destroyed by main thread.
+                    #
+                    return
+                end
 		@@_interrupted = true
+		@@_callbackInProcess = true
 	    }
 
 	    begin
-		@@_communicator.shutdown()
+		@@_application.interruptCallback(sig)
 	    rescue => ex
 		puts $!
-		puts @@_appName + " (while shutting down in response to signal " + sig + "):"
+		puts @@_appName + " (while interrupting in response to signal " + sig + "):"
 		puts ex.backtrace.join("\n")
 	    end
+	    @@_mutex.synchronize {
+		@@_callbackInProcess = false
+		@@_condVar.signal
+	    }
 	end
 
 	@@_appName = nil
 	@@_communicator = nil
+	@@_application = nil
+	@@_ctrlCHandler = nil
+	@@_previousCallback = nil
 	@@_interrupted = false
 	@@_released = false
-	@@_mutex = Mutex.new
+	@@_destroyed = false
+	@@_callbackInProgress = false
 	@@_condVar = ConditionVariable.new
-	@@_ctrlCHandler = nil
-	@@_previousCallback = nil
+	@@_mutex = Mutex.new
 	@@_holdInterruptCallbackProc = Proc.new { |sig| Application::holdInterruptCallback(sig) }
 	@@_destroyOnInterruptCallbackProc = Proc.new { |sig| Application::destroyOnInterruptCallback(sig) }
-	@@_shutdownOnInterruptCallbackProc = Proc.new { |sig| Application::shutdownOnInterruptCallback(sig) }
+	@@_callbackOnInterruptCallbackProc = Proc.new { |sig| Application::callbackOnInterruptCallback(sig) }
     end
 
     #
diff --git a/rb/src/IceRuby/Communicator.cpp b/rb/src/IceRuby/Communicator.cpp
index 9902f7a7b1a..21461c023a3 100644
--- a/rb/src/IceRuby/Communicator.cpp
+++ b/rb/src/IceRuby/Communicator.cpp
@@ -34,9 +34,16 @@ void
 IceRuby_Communicator_mark(Ice::CommunicatorPtr* p)
 {
     assert(p);
-    ObjectFactoryPtr pof = ObjectFactoryPtr::dynamicCast((*p)->findObjectFactory(""));
-    assert(pof);
-    pof->mark();
+    try
+    {
+	ObjectFactoryPtr pof = ObjectFactoryPtr::dynamicCast((*p)->findObjectFactory(""));
+	assert(pof);
+	pof->mark();
+    }
+    catch(const Ice::CommunicatorDestroyedException&)
+    {
+    	// Ignore. This is expected.
+    }
 }
 
 extern "C"
diff --git a/rb/src/IceRuby/Makefile b/rb/src/IceRuby/Makefile
index a2cac771d4d..ff69f075f57 100644
--- a/rb/src/IceRuby/Makefile
+++ b/rb/src/IceRuby/Makefile
@@ -9,8 +9,8 @@
 
 top_srcdir	= ../..
 
-LIBNAME		= $(call mkrubylibname,IceRuby)
-SONAME          = $(LIBNAME)
+LIBNAME		= $(call mklibname,IceRuby)
+SONAME          = $(call mksoname,Ice,$(SOVERSION))  
 
 TARGETS		= $(rubydir)/$(LIBNAME)
 
diff --git a/rb/test/Ice/application/Client.rb b/rb/test/Ice/application/Client.rb
new file mode 100644
index 00000000000..36ff1595bd1
--- /dev/null
+++ b/rb/test/Ice/application/Client.rb
@@ -0,0 +1,47 @@
+#!/usr/bin/env ruby
+# **********************************************************************
+#
+# Copyright (c) 2003-2007 ZeroC, Inc. All rights reserved.
+#
+# This copy of Ice is licensed to you under the terms described in the
+# ICE_LICENSE file included in this distribution.
+#
+# **********************************************************************
+
+require 'Ice'
+
+class Client < Ice::Application
+    def interruptCallback(sig)
+    	puts "handling signal " + sig
+    end
+
+    def run(args)
+    	Ice::Application::ignoreInterrupt
+	puts "Ignore CTRL+C and the like for 5 seconds (try it!)"
+	sleep(5)
+
+    	Ice::Application::callbackOnInterrupt
+
+    	Ice::Application::holdInterrupt
+	puts "Hold CTRL+C and the like for 5 seconds (try it!)"
+	sleep(5)
+
+	Ice::Application::releaseInterrupt
+	puts "Release CTRL+C (any held signals should be released)"
+	sleep(5)
+
+    	Ice::Application::holdInterrupt
+	puts "Hold CTRL+C and the like for 5 seconds (try it!)"
+	sleep(5)
+
+	Ice::Application::callbackOnInterrupt
+	puts "Release CTRL+C (any held signals should be released)"
+	sleep(5)
+
+	puts "ok"
+	return true
+    end
+end
+
+app = Client.new()
+exit(app.main(ARGV))