Skip to content

Commit

Permalink
Fix #23821: Ensure that remote control commands are processed in order
Browse files Browse the repository at this point in the history 
This reverts or partially reverts r19153 and r19196 in favour of forcing ordering
in the `RequestProcessor#run` method. This does not block the server thread, but
it can mean that we have a bunch of processor threads that are waiting on the
previous processor thread.

git-svn-id: https://josm.openstreetmap.de/svn/trunk@19200 0c6e7542-c601-0410-84e7-c038aed88b3b
  • Loading branch information
taylor.smock committed Aug 19, 2024
1 parent 100ec1f commit ffa514d
Show file tree
Hide file tree
Showing 4 changed files with 193 additions and 191 deletions.
206 changes: 125 additions & 81 deletions src/org/openstreetmap/josm/io/remotecontrol/RequestProcessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Optional;
import java.util.StringTokenizer;
import java.util.TreeMap;
import java.util.concurrent.locks.ReentrantLock;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Stream;
Expand Down Expand Up @@ -53,6 +54,8 @@
*/
public class RequestProcessor extends Thread {

/** This is purely used to ensure that remote control commands are executed in the order in which they are received */
private static final ReentrantLock ORDER_LOCK = new ReentrantLock(true);
private static final Charset RESPONSE_CHARSET = StandardCharsets.UTF_8;
private static final String RESPONSE_TEMPLATE = "<!DOCTYPE html><html><head><meta charset=\""
+ RESPONSE_CHARSET.name()
Expand Down Expand Up @@ -181,11 +184,30 @@ public static void initialize() {
*/
@Override
public void run() {
Writer out = null; // NOPMD
try { // NOPMD
out = new OutputStreamWriter(new BufferedOutputStream(request.getOutputStream()), RESPONSE_CHARSET);
BufferedReader in = new BufferedReader(new InputStreamReader(request.getInputStream(), StandardCharsets.US_ASCII)); // NOPMD
// The locks ensure that we process the instructions in the order in which they came.
// This is mostly important when the caller is attempting to create a new layer and add multiple download
// instructions for that layer. See #23821 for additional details.
ORDER_LOCK.lock();
try (request;
Writer out = new OutputStreamWriter(new BufferedOutputStream(request.getOutputStream()), RESPONSE_CHARSET);
BufferedReader in = new BufferedReader(new InputStreamReader(request.getInputStream(), StandardCharsets.US_ASCII))) {
realRun(in, out, request);
} catch (IOException ioe) {
Logging.debug(Logging.getErrorMessage(ioe));
} finally {
ORDER_LOCK.unlock();
}
}

/**
* Perform the actual commands
* @param in The reader for incoming data
* @param out The writer for outgoing data
* @param request The actual request
* @throws IOException Usually occurs if one of the {@link Writer} methods has problems.
*/
private static void realRun(BufferedReader in, Writer out, Socket request) throws IOException {
try {
String get = in.readLine();
if (get == null) {
sendInternalError(out, null);
Expand All @@ -210,94 +232,116 @@ public void run() {
return;
}

int questionPos = url.indexOf('?');
final int questionPos = url.indexOf('?');

String command = questionPos < 0 ? url : url.substring(0, questionPos);
final String command = questionPos < 0 ? url : url.substring(0, questionPos);

Map<String, String> headers = new HashMap<>();
int k = 0;
int maxHeaders = 20;
while (k < maxHeaders) {
get = in.readLine();
if (get == null) break;
final Map<String, String> headers = parseHeaders(in);
final String sender = parseSender(headers, request);
callHandler(url, command, out, sender);
} catch (ReflectiveOperationException e) {
Logging.error(e);
try {
sendInternalError(out, e.getMessage());
} catch (IOException e1) {
Logging.warn(e1);
}
}
}

/**
* Parse the headers from the request
* @param in The request reader
* @return The map of headers
* @throws IOException See {@link BufferedReader#readLine()}
*/
private static Map<String, String> parseHeaders(BufferedReader in) throws IOException {
Map<String, String> headers = new HashMap<>();
int k = 0;
int maxHeaders = 20;
int lastSize = -1;
while (k < maxHeaders && lastSize != headers.size()) {
lastSize = headers.size();
String get = in.readLine();
if (get != null) {
k++;
String[] h = get.split(": ", 2);
if (h.length == 2) {
headers.put(h[0], h[1]);
} else break;
}

// Who sent the request: trying our best to detect
// not from localhost => sender = IP
// from localhost: sender = referer header, if exists
String sender = null;

if (!request.getInetAddress().isLoopbackAddress()) {
sender = request.getInetAddress().getHostAddress();
} else {
String ref = headers.get("Referer");
Pattern r = Pattern.compile("(https?://)?([^/]*)");
if (ref != null) {
Matcher m = r.matcher(ref);
if (m.find()) {
sender = m.group(2);
}
}
if (sender == null) {
sender = "localhost";
}
}
}
return headers;
}

// find a handler for this command
Class<? extends RequestHandler> handlerClass = handlers.get(command);
if (handlerClass == null) {
String usage = getUsageAsHtml();
String websiteDoc = HelpUtil.getWikiBaseHelpUrl() +"/Help/Preferences/RemoteControl";
String help = "No command specified! The following commands are available:<ul>" + usage
+ "</ul>" + "See <a href=\""+websiteDoc+"\">"+websiteDoc+"</a> for complete documentation.";
sendErrorHtml(out, 400, "Bad Request", help);
} else {
// create handler object
RequestHandler handler = handlerClass.getConstructor().newInstance();
try {
handler.setCommand(command);
handler.setUrl(url);
handler.setSender(sender);
handler.handle();
sendHeader(out, "200 OK", handler.getContentType(), false);
out.write("Content-length: " + handler.getContent().length()
+ "\r\n");
out.write("\r\n");
out.write(handler.getContent());
out.flush();
} catch (RequestHandlerOsmApiException ex) {
Logging.debug(ex);
sendBadGateway(out, ex.getMessage());
} catch (RequestHandlerErrorException ex) {
Logging.debug(ex);
sendInternalError(out, ex.getMessage());
} catch (RequestHandlerBadRequestException ex) {
Logging.debug(ex);
sendBadRequest(out, ex.getMessage());
} catch (RequestHandlerForbiddenException ex) {
Logging.debug(ex);
sendForbidden(out, ex.getMessage());
}
}
} catch (IOException ioe) {
Logging.debug(Logging.getErrorMessage(ioe));
} catch (ReflectiveOperationException e) {
Logging.error(e);
try {
sendInternalError(out, e.getMessage());
} catch (IOException e1) {
Logging.warn(e1);
/**
* Attempt to figure out who sent the request
* @param headers The headers (we currently look for {@code Referer})
* @param request The request to look at
* @return The sender (or {@code "localhost"} if none could be found)
*/
private static String parseSender(Map<String, String> headers, Socket request) {
// Who sent the request: trying our best to detect
// not from localhost => sender = IP
// from localhost: sender = referer header, if exists
if (!request.getInetAddress().isLoopbackAddress()) {
return request.getInetAddress().getHostAddress();
}
String ref = headers.get("Referer");
Pattern r = Pattern.compile("(https?://)?([^/]*)");
if (ref != null) {
Matcher m = r.matcher(ref);
if (m.find()) {
return m.group(2);
}
} finally {
}
return "localhost";
}

/**
* Call the handler for the command
* @param url The request URL
* @param command The command we are using
* @param out The writer to use for indicating success or failure
* @param sender The sender of the request
* @throws ReflectiveOperationException If the handler class has an issue
* @throws IOException If one of the {@link Writer} methods has issues
*/
private static void callHandler(String url, String command, Writer out, String sender) throws ReflectiveOperationException, IOException {
// find a handler for this command
Class<? extends RequestHandler> handlerClass = handlers.get(command);
if (handlerClass == null) {
String usage = getUsageAsHtml();
String websiteDoc = HelpUtil.getWikiBaseHelpUrl() +"/Help/Preferences/RemoteControl";
String help = "No command specified! The following commands are available:<ul>" + usage
+ "</ul>" + "See <a href=\""+websiteDoc+"\">"+websiteDoc+"</a> for complete documentation.";
sendErrorHtml(out, 400, "Bad Request", help);
} else {
// create handler object
RequestHandler handler = handlerClass.getConstructor().newInstance();
try {
request.close();
} catch (IOException e) {
Logging.debug(Logging.getErrorMessage(e));
handler.setCommand(command);
handler.setUrl(url);
handler.setSender(sender);
handler.handle();
sendHeader(out, "200 OK", handler.getContentType(), false);
out.write("Content-length: " + handler.getContent().length()
+ "\r\n");
out.write("\r\n");
out.write(handler.getContent());
out.flush();
} catch (RequestHandlerOsmApiException ex) {
Logging.debug(ex);
sendBadGateway(out, ex.getMessage());
} catch (RequestHandlerErrorException ex) {
Logging.debug(ex);
sendInternalError(out, ex.getMessage());
} catch (RequestHandlerBadRequestException ex) {
Logging.debug(ex);
sendBadRequest(out, ex.getMessage());
} catch (RequestHandlerForbiddenException ex) {
Logging.debug(ex);
sendForbidden(out, ex.getMessage());
}
}
}
Expand Down
61 changes: 1 addition & 60 deletions src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import java.awt.geom.Rectangle2D;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand All @@ -16,8 +15,6 @@
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -75,8 +72,6 @@ public class LoadAndZoomHandler extends RequestHandler {
private static final String CHANGESET_TAGS = "changeset_tags";
private static final String SEARCH = "search";

private static final Map<String, ReadWriteLock> layerLockMap = new HashMap<>();

// Mandatory arguments
private double minlat;
private double maxlat;
Expand Down Expand Up @@ -165,19 +160,13 @@ protected void handleRequest() throws RequestHandlerErrorException {

private void download() throws RequestHandlerErrorException {
DownloadOsmTask osmTask = new DownloadOsmTask();
ReadWriteLock lock = null;
DownloadParams settings = null;
try {
settings = getDownloadParams();
final DownloadParams settings = getDownloadParams();

if (command.equals(myCommand)) {
if (!PermissionPrefWithDefault.LOAD_DATA.isAllowed()) {
Logging.info("RemoteControl: download forbidden by preferences");
} else {
// The lock ensures that a download that creates a new layer will finish before
// downloads that do not create a new layer. This should be thread-safe.
lock = obtainLock(settings);
// We need to ensure that we only try to download new areas
Area toDownload = null;
if (!settings.isNewLayer()) {
toDownload = removeAlreadyDownloadedArea();
Expand All @@ -189,58 +178,10 @@ private void download() throws RequestHandlerErrorException {
}
}
}
} catch (InterruptedException ex) {
Thread.currentThread().interrupt();
throw new RequestHandlerErrorException(ex);
} catch (RuntimeException ex) { // NOPMD
Logging.warn("RemoteControl: Error parsing load_and_zoom remote control request:");
Logging.error(ex);
throw new RequestHandlerErrorException(ex);
} finally {
releaseLock(settings, lock);
}
}

/**
* Obtain a lock to ensure that a new layer is created before downloading non-new layers
* @param settings The settings with the appropriate layer name; if no layer name is given, we assume that
* the caller doesn't care where the data goes.
* @return The lock to pass to {@link #releaseLock(DownloadParams, ReadWriteLock)} or {@code null} if no lock is needed.
* @throws InterruptedException If the lock could not be obtained.
*/
private static ReadWriteLock obtainLock(DownloadParams settings) throws InterruptedException {
final ReadWriteLock lock;
if (settings.isNewLayer() && !Utils.isEmpty(settings.getLayerName())) {
synchronized (layerLockMap) {
lock = layerLockMap.computeIfAbsent(settings.getLayerName(), k -> new ReentrantReadWriteLock());
lock.writeLock().lock();
}
} else {
synchronized (layerLockMap) {
lock = layerLockMap.get(settings.getLayerName());
}
if (lock != null) {
lock.readLock().lockInterruptibly();
}
}
return lock;
}

/**
* Release the lock preventing data from being downloaded into an old layer
* @param settings The settings with information on the new layer status
* @param lock The lock to unlock
*/
private static void releaseLock(DownloadParams settings, ReadWriteLock lock) {
if (lock != null) {
if (settings != null && settings.isNewLayer()) {
lock.writeLock().unlock();
synchronized (layerLockMap) {
layerLockMap.remove(settings.getLayerName());
}
} else {
lock.readLock().unlock();
}
}
}

Expand Down
Loading

0 comments on commit ffa514d

Please sign in to comment.