Index: trunk/src/org/openstreetmap/josm/io/remotecontrol/RequestProcessor.java =================================================================== --- trunk/src/org/openstreetmap/josm/io/remotecontrol/RequestProcessor.java (revision 19199) +++ trunk/src/org/openstreetmap/josm/io/remotecontrol/RequestProcessor.java (revision 19200) @@ -21,4 +21,5 @@ import java.util.StringTokenizer; import java.util.TreeMap; +import java.util.concurrent.locks.ReentrantLock; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -54,4 +55,6 @@ 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 = " headers = new HashMap<>(); - int k = 0; - int maxHeaders = 20; - while (k < maxHeaders) { - get = in.readLine(); - if (get == null) break; - 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"; - } - } - - // find a handler for this command - Class 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:" + "See "+websiteDoc+" 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)); + final int questionPos = url.indexOf('?'); + + final String command = questionPos < 0 ? url : url.substring(0, questionPos); + + final Map headers = parseHeaders(in); + final String sender = parseSender(headers, request); + callHandler(url, command, out, sender); } catch (ReflectiveOperationException e) { Logging.error(e); @@ -294,9 +247,100 @@ Logging.warn(e1); } - } finally { + } + } + + /** + * 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 parseHeaders(BufferedReader in) throws IOException { + Map 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]); + } + } + } + return headers; + } + + /** + * 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 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); + } + } + 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 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:
    " + usage + + "
" + "See "+websiteDoc+" 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()); } } Index: trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java =================================================================== --- trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19199) +++ trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java (revision 19200) @@ -8,5 +8,4 @@ import java.util.Collection; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; @@ -17,6 +16,4 @@ 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; @@ -76,6 +73,4 @@ private static final String SEARCH = "search"; - private static final Map layerLockMap = new HashMap<>(); - // Mandatory arguments private double minlat; @@ -166,8 +161,6 @@ 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)) { @@ -175,8 +168,4 @@ 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()) { @@ -190,56 +179,8 @@ } } - } 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(); - } } } Index: trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/RequestHandler.java =================================================================== --- trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/RequestHandler.java (revision 19199) +++ trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/RequestHandler.java (revision 19200) @@ -14,5 +14,4 @@ import java.util.Map; import java.util.Set; -import java.util.concurrent.locks.ReentrantLock; import java.util.function.Function; import java.util.function.Supplier; @@ -49,6 +48,4 @@ /** preference to define OSM download timeout in seconds */ public static final IntegerProperty OSM_DOWNLOAD_TIMEOUT = new IntegerProperty("remotecontrol.osm.download.timeout", 5*60); - /** A lock to ensure that messages are shown in the order the commands were sent from the server (see #23821) */ - private static final ReentrantLock MESSAGE_LOCK = new ReentrantLock(true); protected static final Pattern SPLITTER_COMMA = Pattern.compile(",\\s*"); @@ -212,24 +209,17 @@ final Object[] choices = {tr("Yes, always"), tr("Yes, once"), tr("No")}; final int choice; - // The ordering of the requests can be important, so we use a fair lock to ensure ordering - // Note that the EDT will be more than happy to show multiple dialogs without blocking. - try { - MESSAGE_LOCK.lock(); - final Integer tChoice = GuiHelper.runInEDTAndWaitAndReturn(() -> { - final JLabel label = new JLabel(message); - if (label.getPreferredSize().width > maxWidth) { - label.setText(message.replaceFirst("
", "
")); - } - return JOptionPane.showOptionDialog(MainApplication.getMainFrame(), label, tr("Confirm Remote Control action"), - JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.QUESTION_MESSAGE, null, choices, choices[1]); - }); - if (tChoice == null) { - // I have no clue how this would ever happen, but just in case. - throw new RequestHandlerForbiddenException(MessageFormat.format("RemoteControl: ''{0}'' forbidden due to NPE", myCommand)); + final Integer tChoice = GuiHelper.runInEDTAndWaitAndReturn(() -> { + final JLabel label = new JLabel(message); + if (label.getPreferredSize().width > maxWidth) { + label.setText(message.replaceFirst("
", "
")); } - choice = tChoice; - } finally { - MESSAGE_LOCK.unlock(); + return JOptionPane.showOptionDialog(MainApplication.getMainFrame(), label, tr("Confirm Remote Control action"), + JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.QUESTION_MESSAGE, null, choices, choices[1]); + }); + if (tChoice == null) { + // I have no clue how this would ever happen, but just in case. + throw new RequestHandlerForbiddenException(MessageFormat.format("RemoteControl: ''{0}'' forbidden due to NPE", myCommand)); } + choice = tChoice; if (choice != JOptionPane.YES_OPTION && choice != JOptionPane.NO_OPTION) { // Yes/no refer to always/once String err = MessageFormat.format("RemoteControl: ''{0}'' forbidden by user''s choice", myCommand);