Bug 485234 - [http whiteboard] FilterRegistration objects are not

cleaned up properly when unregistered

Change-Id: I45139c151a34a254e9347d1ac430026a60158018
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

2 files changed, 42 insertions, 7 deletions

diff --git a/bundles/org.eclipse.equinox.http.servlet.tests/src/org/eclipse/equinox/http/servlet/tests/ServletTest.java b/bundles/org.eclipse.equinox.http.servlet.tests/src/org/eclipse/equinox/http/servlet/tests/ServletTest.java
index 8727d4106..ac45f1e6e 100644
--- a/bundles/org.eclipse.equinox.http.servlet.tests/src/org/eclipse/equinox/http/servlet/tests/ServletTest.java
+++ b/bundles/org.eclipse.equinox.http.servlet.tests/src/org/eclipse/equinox/http/servlet/tests/ServletTest.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011, 2015 IBM Corporation and others.
+ * Copyright (c) 2011, 2016 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -949,9 +949,9 @@ public class ServletTest extends TestCase {
 		TestFilter testFilter2 = new TestFilter();
 		Servlet testServlet = new BaseServlet(expected);
 
-		getBundleContext().registerService(Filter.class, testFilter1, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, "/*")));
-		getBundleContext().registerService(Filter.class, testFilter2, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, "/hello/*")));
-		getBundleContext().registerService(Servlet.class, testServlet, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "/")));
+		registrations.add(getBundleContext().registerService(Filter.class, testFilter1, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, "/*"))));
+		registrations.add(getBundleContext().registerService(Filter.class, testFilter2, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, "/hello/*"))));
+		registrations.add(getBundleContext().registerService(Servlet.class, testServlet, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "/"))));
 
 		String actual = requestAdvisor.request("hello_test/request");
 		Assert.assertEquals(expected, actual);
@@ -964,6 +964,38 @@ public class ServletTest extends TestCase {
 		Assert.assertTrue("testFilter2 did not get called.", testFilter2.getCalled());
 	}
 
+	public void test_Filter24() throws Exception {
+		// Test WB servlet and WB testfilter matching against it.
+		// Test filter gets called.
+		// Unregister WB filter.
+		// test filter is NOT called
+		String expected = "a";
+		TestFilter testFilter1 = new TestFilter();
+		Servlet testServlet = new BaseServlet(expected);
+
+		ServiceRegistration<Filter> filterReg = getBundleContext().registerService(Filter.class, testFilter1, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_FILTER_PATTERN, "/hello/*")));
+		try {
+			registrations.add(getBundleContext().registerService(Servlet.class, testServlet, new Hashtable<String, String>(Collections.singletonMap(HttpWhiteboardConstants.HTTP_WHITEBOARD_SERVLET_PATTERN, "/hello/*"))));
+
+			String actual = requestAdvisor.request("hello/request");
+			Assert.assertEquals(expected, actual);
+			Assert.assertTrue("testFilter1 did not get called.", testFilter1.getCalled());
+
+			filterReg.unregister();
+			filterReg = null;
+			testFilter1.clear();
+
+			actual = requestAdvisor.request("hello/request");
+			Assert.assertEquals(expected, actual);
+			Assert.assertFalse("testFilter1 did get called.", testFilter1.getCalled());
+		} finally {
+			if (filterReg != null) {
+				filterReg.unregister();
+			}
+		}
+	}
+
+
 	public void test_Registration1() throws Exception {
 		String expected = "Alias cannot be null";
 		try {
diff --git a/bundles/org.eclipse.equinox.http.servlet/src/org/eclipse/equinox/http/servlet/internal/registration/FilterRegistration.java b/bundles/org.eclipse.equinox.http.servlet/src/org/eclipse/equinox/http/servlet/internal/registration/FilterRegistration.java
index 70f12ee78..27975ca81 100644
--- a/bundles/org.eclipse.equinox.http.servlet/src/org/eclipse/equinox/http/servlet/internal/registration/FilterRegistration.java
+++ b/bundles/org.eclipse.equinox.http.servlet/src/org/eclipse/equinox/http/servlet/internal/registration/FilterRegistration.java
@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2011, 2015 IBM Corporation and others.
+ * Copyright (c) 2011, 2016 IBM Corporation and others.
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -77,8 +77,11 @@ public class FilterRegistration
 		if (priorityDifference != 0)
 			return -priorityDifference;
 
-		return (Math.abs(getD().serviceId) >
-			Math.abs(otherFilterRegistration.getD().serviceId)) ? 1 : -1;
+		// Note that we use abs here because the DTO service ID may have been negated for legacy filters.
+		// We always compare with the positive id values and we know the positive values are unique.
+		long thisId = Math.abs(getD().serviceId);
+		long otherId = Math.abs(otherFilterRegistration.getD().serviceId);
+		return (thisId < otherId) ? -1 : ((thisId == otherId) ? 0 : 1);
 	}
 
 	public void destroy() {