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
This commit is contained in:
Simone Bordet
parent
b16c1f00dc
commit
0b70620b82
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 ||
|
||||
|
|
|
|||
|
|
@ -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<Exception> failure = new AtomicReference<Exception>();
|
||||
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<Exception> failure;
|
||||
|
||||
private TestHttpExchange(ExecutorService executor, AtomicReference<Exception> failure)
|
||||
{
|
||||
this.executor = executor;
|
||||
this.failure = failure;
|
||||
}
|
||||
|
||||
@Override
|
||||
protected void onRequestCommitted() throws IOException
|
||||
{
|
||||
Future<Integer> future = executor.submit(new Callable<Integer>()
|
||||
{
|
||||
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);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue