From 0b70620b8282cb0105dd5c8487104b36c68457a2 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 17 Sep 2009 08:03:08 +0000 Subject: [PATCH] Fixed bug #289686: HttpExchange.setStatus() has too coarse synchronization. git-svn-id: svn+ssh://dev.eclipse.org/svnroot/rt/org.eclipse.jetty/jetty/trunk@929 7e9141cc-0065-0410-87d8-b60c137991c4 --- VERSION.txt | 3 +- .../eclipse/jetty/client/HttpExchange.java | 329 +++++++++--------- .../client/AsyncCallbackHttpExchangeTest.java | 97 ++++++ 3 files changed, 266 insertions(+), 163 deletions(-) create mode 100644 jetty-client/src/test/java/org/eclipse/jetty/client/AsyncCallbackHttpExchangeTest.java diff --git a/VERSION.txt b/VERSION.txt index 2f6c7b14487..c98e2d3fade 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -5,7 +5,7 @@ jetty-7.0.1-SNAPSHOT + 288401 HttpExchange.cancel() Method Unimplemented + 289265 Test harness for async input + 289027 deobfuscate HttpClient SSL passwords - + jetty-7.0.0.RC6-SNAPSHOT + JETTY-719 Document state machine of jetty http client + JETTY-780 CNFE during startup of webapp with spring-context >= 2.5.1 @@ -37,6 +37,7 @@ jetty-7.0.0.RC6-SNAPSHOT + 289285 org.eclipse.jetty.continuation 7.0.0.RC5 imports the org.mortbay.util.ajax package + Tweak DefaultServletTest under windows + Copy VERSION.txt to distro + + 289686 HttpExchange.setStatus() has too coarse synchronization jetty-6.1.20 27 August 2009 + JETTY-838 Don't log and throw diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java index 13787d7eb1d..8fa02fc7306 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpExchange.java @@ -132,172 +132,168 @@ public class HttpExchange setStatus(STATUS_START); } - void setStatus(int status) + void setStatus(int newStatus) { try { - synchronized (this) + int oldStatus = getStatus(); + + // State machine: from which old status you can go into which new status + switch (oldStatus) { - // Wakeup any waiter, no matter if the status is really changed - notifyAll(); - - // State machine: from which old status you can go into which new status - switch (_status) - { - case STATUS_START: - switch (status) - { - case STATUS_WAITING_FOR_CONNECTION: - case STATUS_WAITING_FOR_COMMIT: - case STATUS_CANCELLING: - _status = status; - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_WAITING_FOR_CONNECTION: - switch (status) - { - case STATUS_WAITING_FOR_COMMIT: - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_WAITING_FOR_COMMIT: - switch (status) - { - case STATUS_SENDING_REQUEST: - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - case STATUS_EXPIRED: - _status = status; - getEventListener().onExpire(); - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_SENDING_REQUEST: - switch (status) - { - case STATUS_WAITING_FOR_RESPONSE: - _status = status; - getEventListener().onRequestCommitted(); - break; - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - case STATUS_EXPIRED: - _status = status; - getEventListener().onExpire(); - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_WAITING_FOR_RESPONSE: - switch (status) - { - case STATUS_PARSING_HEADERS: - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - case STATUS_EXPIRED: - _status = status; - getEventListener().onExpire(); - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_PARSING_HEADERS: - switch (status) - { - case STATUS_PARSING_CONTENT: - _status = status; - getEventListener().onResponseHeaderComplete(); - break; - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - case STATUS_EXPIRED: - _status = status; - getEventListener().onExpire(); - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_PARSING_CONTENT: - switch (status) - { - case STATUS_COMPLETED: - _status = status; - getEventListener().onResponseComplete(); - break; - case STATUS_CANCELLING: - case STATUS_EXCEPTED: - _status = status; - break; - case STATUS_EXPIRED: - _status = status; - getEventListener().onExpire(); - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_COMPLETED: - switch (status) - { - case STATUS_START: - _status = status; - break; - case STATUS_CANCELLING: - case STATUS_EXPIRED: - // Don't change the status, it's too late - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - case STATUS_CANCELLING: - switch (status) - { - case STATUS_CANCELLED: - _status = status; - break; - default: - // Ignore other statuses, we're cancelling - break; - } - break; - case STATUS_EXCEPTED: - case STATUS_EXPIRED: - case STATUS_CANCELLED: - switch (status) - { - case STATUS_START: - _status = status; - break; - default: - throw new IllegalStateException(_status + " => " + status); - } - break; - default: - // Here means I allowed to set a state that I don't recognize - throw new AssertionError(_status + " => " + status); - } + case STATUS_START: + switch (newStatus) + { + case STATUS_WAITING_FOR_CONNECTION: + case STATUS_WAITING_FOR_COMMIT: + case STATUS_CANCELLING: + updateStatus(newStatus); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_WAITING_FOR_CONNECTION: + switch (newStatus) + { + case STATUS_WAITING_FOR_COMMIT: + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_WAITING_FOR_COMMIT: + switch (newStatus) + { + case STATUS_SENDING_REQUEST: + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + case STATUS_EXPIRED: + updateStatus(newStatus); + getEventListener().onExpire(); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_SENDING_REQUEST: + switch (newStatus) + { + case STATUS_WAITING_FOR_RESPONSE: + updateStatus(newStatus); + getEventListener().onRequestCommitted(); + break; + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + case STATUS_EXPIRED: + updateStatus(newStatus); + getEventListener().onExpire(); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_WAITING_FOR_RESPONSE: + switch (newStatus) + { + case STATUS_PARSING_HEADERS: + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + case STATUS_EXPIRED: + updateStatus(newStatus); + getEventListener().onExpire(); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_PARSING_HEADERS: + switch (newStatus) + { + case STATUS_PARSING_CONTENT: + updateStatus(newStatus); + getEventListener().onResponseHeaderComplete(); + break; + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + case STATUS_EXPIRED: + updateStatus(newStatus); + getEventListener().onExpire(); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_PARSING_CONTENT: + switch (newStatus) + { + case STATUS_COMPLETED: + updateStatus(newStatus); + getEventListener().onResponseComplete(); + break; + case STATUS_CANCELLING: + case STATUS_EXCEPTED: + updateStatus(newStatus); + break; + case STATUS_EXPIRED: + updateStatus(newStatus); + getEventListener().onExpire(); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_COMPLETED: + switch (newStatus) + { + case STATUS_START: + updateStatus(newStatus); + break; + case STATUS_CANCELLING: + case STATUS_EXPIRED: + // Don't change the status, it's too late + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + case STATUS_CANCELLING: + switch (newStatus) + { + case STATUS_CANCELLED: + updateStatus(newStatus); + break; + default: + // Ignore other statuses, we're cancelling + break; + } + break; + case STATUS_EXCEPTED: + case STATUS_EXPIRED: + case STATUS_CANCELLED: + switch (newStatus) + { + case STATUS_START: + updateStatus(newStatus); + break; + default: + throw new IllegalStateException(oldStatus + " => " + newStatus); + } + break; + default: + // Here means I allowed to set a state that I don't recognize + throw new AssertionError(oldStatus + " => " + newStatus); } } catch (IOException x) @@ -306,6 +302,15 @@ public class HttpExchange } } + private void updateStatus(int newStatus) + { + synchronized (this) + { + _status = newStatus; + notifyAll(); + } + } + public boolean isDone (int status) { return status == STATUS_COMPLETED || diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/AsyncCallbackHttpExchangeTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/AsyncCallbackHttpExchangeTest.java new file mode 100644 index 00000000000..85a8cb14be6 --- /dev/null +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/AsyncCallbackHttpExchangeTest.java @@ -0,0 +1,97 @@ +/* + * Copyright (c) 2009-2009 Mort Bay Consulting Pty. Ltd. + * + * All rights reserved. This program and the accompanying materials + * are made available under the terms of the Eclipse Public License v1.0 + * and Apache License v2.0 which accompanies this distribution. + * The Eclipse Public License is available at + * http://www.eclipse.org/legal/epl-v10.html + * The Apache License v2.0 is available at + * http://www.opensource.org/licenses/apache2.0.php + * + * You may elect to redistribute this code under either of these licenses. + */ + +package org.eclipse.jetty.client; + +import java.io.IOException; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import junit.framework.TestCase; + +/** + * @version $Revision$ $Date$ + */ +public class AsyncCallbackHttpExchangeTest extends TestCase +{ + /** + * If the HttpExchange callbacks are called holding the lock on HttpExchange, + * it will be impossible for the callback to perform some work asynchronously + * and contemporarly accessing the HttpExchange instance synchronized state. + * This test verifies that this situation does not happen. + * + * @throws Exception if the test fails + */ + public void testAsyncCallback() throws Exception + { + ExecutorService executor = Executors.newCachedThreadPool(); + try + { + AtomicReference failure = new AtomicReference(); + TestHttpExchange exchange = new TestHttpExchange(executor, failure); + exchange.setStatus(HttpExchange.STATUS_WAITING_FOR_COMMIT); + exchange.setStatus(HttpExchange.STATUS_SENDING_REQUEST); + // This status change triggers onRequestCommitted() + exchange.setStatus(HttpExchange.STATUS_WAITING_FOR_RESPONSE); + assertNull(failure.get()); + } + finally + { + executor.shutdown(); + } + } + + private class TestHttpExchange extends HttpExchange + { + private final ExecutorService executor; + private final AtomicReference failure; + + private TestHttpExchange(ExecutorService executor, AtomicReference failure) + { + this.executor = executor; + this.failure = failure; + } + + @Override + protected void onRequestCommitted() throws IOException + { + Future future = executor.submit(new Callable() + { + public Integer call() throws Exception + { + // Method getStatus() reads synchronized state + return TestHttpExchange.this.getStatus(); + } + }); + + // We're waiting for the future to complete, thus never exiting + // this method; if this method is called with the lock held, + // this method never completes + try + { + future.get(1000, TimeUnit.MILLISECONDS); + // Test green here + } + catch (Exception x) + { + // Timed out, the test did not pass + failure.set(x); + } + } + } +}