Changeset 19200 in josm for trunk/src/org/openstreetmap


Ignore:
Timestamp:
2024-08-19T20:16:02+02:00 (6 months ago)
Author:
taylor.smock
Message:

Fix #23821: Ensure that remote control commands are processed in order

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.

Location:
trunk/src/org/openstreetmap/josm/io/remotecontrol
Files:
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/src/org/openstreetmap/josm/io/remotecontrol/RequestProcessor.java

    r18723 r19200  
    2121import java.util.StringTokenizer;
    2222import java.util.TreeMap;
     23import java.util.concurrent.locks.ReentrantLock;
    2324import java.util.regex.Matcher;
    2425import java.util.regex.Pattern;
     
    5455public class RequestProcessor extends Thread {
    5556
     57    /** This is purely used to ensure that remote control commands are executed in the order in which they are received */
     58    private static final ReentrantLock ORDER_LOCK = new ReentrantLock(true);
    5659    private static final Charset RESPONSE_CHARSET = StandardCharsets.UTF_8;
    5760    private static final String RESPONSE_TEMPLATE = "<!DOCTYPE html><html><head><meta charset=\""
     
    182185    @Override
    183186    public void run() {
    184         Writer out = null; // NOPMD
    185         try { // NOPMD
    186             out = new OutputStreamWriter(new BufferedOutputStream(request.getOutputStream()), RESPONSE_CHARSET);
    187             BufferedReader in = new BufferedReader(new InputStreamReader(request.getInputStream(), StandardCharsets.US_ASCII)); // NOPMD
    188 
     187        // The locks ensure that we process the instructions in the order in which they came.
     188        // This is mostly important when the caller is attempting to create a new layer and add multiple download
     189        // instructions for that layer. See #23821 for additional details.
     190        ORDER_LOCK.lock();
     191        try (request;
     192             Writer out = new OutputStreamWriter(new BufferedOutputStream(request.getOutputStream()), RESPONSE_CHARSET);
     193            BufferedReader in = new BufferedReader(new InputStreamReader(request.getInputStream(), StandardCharsets.US_ASCII))) {
     194            realRun(in, out, request);
     195        } catch (IOException ioe) {
     196            Logging.debug(Logging.getErrorMessage(ioe));
     197        } finally {
     198            ORDER_LOCK.unlock();
     199        }
     200    }
     201
     202    /**
     203     * Perform the actual commands
     204     * @param in The reader for incoming data
     205     * @param out The writer for outgoing data
     206     * @param request The actual request
     207     * @throws IOException Usually occurs if one of the {@link Writer} methods has problems.
     208     */
     209    private static void realRun(BufferedReader in, Writer out, Socket request) throws IOException {
     210        try {
    189211            String get = in.readLine();
    190212            if (get == null) {
     
    211233            }
    212234
    213             int questionPos = url.indexOf('?');
    214 
    215             String command = questionPos < 0 ? url : url.substring(0, questionPos);
    216 
    217             Map<String, String> headers = new HashMap<>();
    218             int k = 0;
    219             int maxHeaders = 20;
    220             while (k < maxHeaders) {
    221                 get = in.readLine();
    222                 if (get == null) break;
    223                 k++;
    224                 String[] h = get.split(": ", 2);
    225                 if (h.length == 2) {
    226                     headers.put(h[0], h[1]);
    227                 } else break;
    228             }
    229 
    230             // Who sent the request: trying our best to detect
    231             // not from localhost => sender = IP
    232             // from localhost: sender = referer header, if exists
    233             String sender = null;
    234 
    235             if (!request.getInetAddress().isLoopbackAddress()) {
    236                 sender = request.getInetAddress().getHostAddress();
    237             } else {
    238                 String ref = headers.get("Referer");
    239                 Pattern r = Pattern.compile("(https?://)?([^/]*)");
    240                 if (ref != null) {
    241                     Matcher m = r.matcher(ref);
    242                     if (m.find()) {
    243                         sender = m.group(2);
    244                     }
    245                 }
    246                 if (sender == null) {
    247                     sender = "localhost";
    248                 }
    249             }
    250 
    251             // find a handler for this command
    252             Class<? extends RequestHandler> handlerClass = handlers.get(command);
    253             if (handlerClass == null) {
    254                 String usage = getUsageAsHtml();
    255                 String websiteDoc = HelpUtil.getWikiBaseHelpUrl() +"/Help/Preferences/RemoteControl";
    256                 String help = "No command specified! The following commands are available:<ul>" + usage
    257                         + "</ul>" + "See <a href=\""+websiteDoc+"\">"+websiteDoc+"</a> for complete documentation.";
    258                 sendErrorHtml(out, 400, "Bad Request", help);
    259             } else {
    260                 // create handler object
    261                 RequestHandler handler = handlerClass.getConstructor().newInstance();
    262                 try {
    263                     handler.setCommand(command);
    264                     handler.setUrl(url);
    265                     handler.setSender(sender);
    266                     handler.handle();
    267                     sendHeader(out, "200 OK", handler.getContentType(), false);
    268                     out.write("Content-length: " + handler.getContent().length()
    269                             + "\r\n");
    270                     out.write("\r\n");
    271                     out.write(handler.getContent());
    272                     out.flush();
    273                 } catch (RequestHandlerOsmApiException ex) {
    274                     Logging.debug(ex);
    275                     sendBadGateway(out, ex.getMessage());
    276                 } catch (RequestHandlerErrorException ex) {
    277                     Logging.debug(ex);
    278                     sendInternalError(out, ex.getMessage());
    279                 } catch (RequestHandlerBadRequestException ex) {
    280                     Logging.debug(ex);
    281                     sendBadRequest(out, ex.getMessage());
    282                 } catch (RequestHandlerForbiddenException ex) {
    283                     Logging.debug(ex);
    284                     sendForbidden(out, ex.getMessage());
    285                 }
    286             }
    287         } catch (IOException ioe) {
    288             Logging.debug(Logging.getErrorMessage(ioe));
     235            final int questionPos = url.indexOf('?');
     236
     237            final String command = questionPos < 0 ? url : url.substring(0, questionPos);
     238
     239            final Map<String, String> headers = parseHeaders(in);
     240            final String sender = parseSender(headers, request);
     241            callHandler(url, command, out, sender);
    289242        } catch (ReflectiveOperationException e) {
    290243            Logging.error(e);
     
    294247                Logging.warn(e1);
    295248            }
    296         } finally {
     249        }
     250    }
     251
     252    /**
     253     * Parse the headers from the request
     254     * @param in The request reader
     255     * @return The map of headers
     256     * @throws IOException See {@link BufferedReader#readLine()}
     257     */
     258    private static Map<String, String> parseHeaders(BufferedReader in) throws IOException {
     259        Map<String, String> headers = new HashMap<>();
     260        int k = 0;
     261        int maxHeaders = 20;
     262        int lastSize = -1;
     263        while (k < maxHeaders && lastSize != headers.size()) {
     264            lastSize = headers.size();
     265            String get = in.readLine();
     266            if (get != null) {
     267                k++;
     268                String[] h = get.split(": ", 2);
     269                if (h.length == 2) {
     270                    headers.put(h[0], h[1]);
     271                }
     272            }
     273        }
     274        return headers;
     275    }
     276
     277    /**
     278     * Attempt to figure out who sent the request
     279     * @param headers The headers (we currently look for {@code Referer})
     280     * @param request The request to look at
     281     * @return The sender (or {@code "localhost"} if none could be found)
     282     */
     283    private static String parseSender(Map<String, String> headers, Socket request) {
     284        // Who sent the request: trying our best to detect
     285        // not from localhost => sender = IP
     286        // from localhost: sender = referer header, if exists
     287        if (!request.getInetAddress().isLoopbackAddress()) {
     288            return request.getInetAddress().getHostAddress();
     289        }
     290        String ref = headers.get("Referer");
     291        Pattern r = Pattern.compile("(https?://)?([^/]*)");
     292        if (ref != null) {
     293            Matcher m = r.matcher(ref);
     294            if (m.find()) {
     295                return m.group(2);
     296            }
     297        }
     298        return "localhost";
     299    }
     300
     301    /**
     302     * Call the handler for the command
     303     * @param url The request URL
     304     * @param command The command we are using
     305     * @param out The writer to use for indicating success or failure
     306     * @param sender The sender of the request
     307     * @throws ReflectiveOperationException If the handler class has an issue
     308     * @throws IOException If one of the {@link Writer} methods has issues
     309     */
     310    private static void callHandler(String url, String command, Writer out, String sender) throws ReflectiveOperationException, IOException {
     311        // find a handler for this command
     312        Class<? extends RequestHandler> handlerClass = handlers.get(command);
     313        if (handlerClass == null) {
     314            String usage = getUsageAsHtml();
     315            String websiteDoc = HelpUtil.getWikiBaseHelpUrl() +"/Help/Preferences/RemoteControl";
     316            String help = "No command specified! The following commands are available:<ul>" + usage
     317                    + "</ul>" + "See <a href=\""+websiteDoc+"\">"+websiteDoc+"</a> for complete documentation.";
     318            sendErrorHtml(out, 400, "Bad Request", help);
     319        } else {
     320            // create handler object
     321            RequestHandler handler = handlerClass.getConstructor().newInstance();
    297322            try {
    298                 request.close();
    299             } catch (IOException e) {
    300                 Logging.debug(Logging.getErrorMessage(e));
     323                handler.setCommand(command);
     324                handler.setUrl(url);
     325                handler.setSender(sender);
     326                handler.handle();
     327                sendHeader(out, "200 OK", handler.getContentType(), false);
     328                out.write("Content-length: " + handler.getContent().length()
     329                        + "\r\n");
     330                out.write("\r\n");
     331                out.write(handler.getContent());
     332                out.flush();
     333            } catch (RequestHandlerOsmApiException ex) {
     334                Logging.debug(ex);
     335                sendBadGateway(out, ex.getMessage());
     336            } catch (RequestHandlerErrorException ex) {
     337                Logging.debug(ex);
     338                sendInternalError(out, ex.getMessage());
     339            } catch (RequestHandlerBadRequestException ex) {
     340                Logging.debug(ex);
     341                sendBadRequest(out, ex.getMessage());
     342            } catch (RequestHandlerForbiddenException ex) {
     343                Logging.debug(ex);
     344                sendForbidden(out, ex.getMessage());
    301345            }
    302346        }
  • trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/LoadAndZoomHandler.java

    r19153 r19200  
    88import java.util.Collection;
    99import java.util.Collections;
    10 import java.util.HashMap;
    1110import java.util.LinkedHashSet;
    1211import java.util.List;
     
    1716import java.util.concurrent.TimeUnit;
    1817import java.util.concurrent.TimeoutException;
    19 import java.util.concurrent.locks.ReadWriteLock;
    20 import java.util.concurrent.locks.ReentrantReadWriteLock;
    2118import java.util.stream.Collectors;
    2219import java.util.stream.Stream;
     
    7673    private static final String SEARCH = "search";
    7774
    78     private static final Map<String, ReadWriteLock> layerLockMap = new HashMap<>();
    79 
    8075    // Mandatory arguments
    8176    private double minlat;
     
    166161    private void download() throws RequestHandlerErrorException {
    167162        DownloadOsmTask osmTask = new DownloadOsmTask();
    168         ReadWriteLock lock = null;
    169         DownloadParams settings = null;
    170163        try {
    171             settings = getDownloadParams();
     164            final DownloadParams settings = getDownloadParams();
    172165
    173166            if (command.equals(myCommand)) {
     
    175168                    Logging.info("RemoteControl: download forbidden by preferences");
    176169                } else {
    177                     // The lock ensures that a download that creates a new layer will finish before
    178                     // downloads that do not create a new layer. This should be thread-safe.
    179                     lock = obtainLock(settings);
    180                     // We need to ensure that we only try to download new areas
    181170                    Area toDownload = null;
    182171                    if (!settings.isNewLayer()) {
     
    190179                }
    191180            }
    192         } catch (InterruptedException ex) {
    193             Thread.currentThread().interrupt();
    194             throw new RequestHandlerErrorException(ex);
    195181        } catch (RuntimeException ex) { // NOPMD
    196182            Logging.warn("RemoteControl: Error parsing load_and_zoom remote control request:");
    197183            Logging.error(ex);
    198184            throw new RequestHandlerErrorException(ex);
    199         } finally {
    200             releaseLock(settings, lock);
    201         }
    202     }
    203 
    204     /**
    205      * Obtain a lock to ensure that a new layer is created before downloading non-new layers
    206      * @param settings The settings with the appropriate layer name; if no layer name is given, we assume that
    207      *                 the caller doesn't care where the data goes.
    208      * @return The lock to pass to {@link #releaseLock(DownloadParams, ReadWriteLock)} or {@code null} if no lock is needed.
    209      * @throws InterruptedException If the lock could not be obtained.
    210      */
    211     private static ReadWriteLock obtainLock(DownloadParams settings) throws InterruptedException {
    212         final ReadWriteLock lock;
    213         if (settings.isNewLayer() && !Utils.isEmpty(settings.getLayerName())) {
    214             synchronized (layerLockMap) {
    215                 lock = layerLockMap.computeIfAbsent(settings.getLayerName(), k -> new ReentrantReadWriteLock());
    216                 lock.writeLock().lock();
    217             }
    218         } else {
    219             synchronized (layerLockMap) {
    220                 lock = layerLockMap.get(settings.getLayerName());
    221             }
    222             if (lock != null) {
    223                 lock.readLock().lockInterruptibly();
    224             }
    225         }
    226         return lock;
    227     }
    228 
    229     /**
    230      * Release the lock preventing data from being downloaded into an old layer
    231      * @param settings The settings with information on the new layer status
    232      * @param lock The lock to unlock
    233      */
    234     private static void releaseLock(DownloadParams settings, ReadWriteLock lock) {
    235         if (lock != null) {
    236             if (settings != null && settings.isNewLayer()) {
    237                 lock.writeLock().unlock();
    238                 synchronized (layerLockMap) {
    239                     layerLockMap.remove(settings.getLayerName());
    240                 }
    241             } else {
    242                 lock.readLock().unlock();
    243             }
    244185        }
    245186    }
  • trunk/src/org/openstreetmap/josm/io/remotecontrol/handler/RequestHandler.java

    r19196 r19200  
    1414import java.util.Map;
    1515import java.util.Set;
    16 import java.util.concurrent.locks.ReentrantLock;
    1716import java.util.function.Function;
    1817import java.util.function.Supplier;
     
    4948    /** preference to define OSM download timeout in seconds */
    5049    public static final IntegerProperty OSM_DOWNLOAD_TIMEOUT = new IntegerProperty("remotecontrol.osm.download.timeout", 5*60);
    51     /** A lock to ensure that messages are shown in the order the commands were sent from the server (see #23821) */
    52     private static final ReentrantLock MESSAGE_LOCK = new ReentrantLock(true);
    5350
    5451    protected static final Pattern SPLITTER_COMMA = Pattern.compile(",\\s*");
     
    212209            final Object[] choices = {tr("Yes, always"), tr("Yes, once"), tr("No")};
    213210            final int choice;
    214             // The ordering of the requests can be important, so we use a fair lock to ensure ordering
    215             // Note that the EDT will be more than happy to show multiple dialogs without blocking.
    216             try {
    217                 MESSAGE_LOCK.lock();
    218                 final Integer tChoice = GuiHelper.runInEDTAndWaitAndReturn(() -> {
    219                     final JLabel label = new JLabel(message);
    220                     if (label.getPreferredSize().width > maxWidth) {
    221                         label.setText(message.replaceFirst("<div>", "<div style=\"width:" + maxWidth + "px;\">"));
    222                     }
    223                     return JOptionPane.showOptionDialog(MainApplication.getMainFrame(), label, tr("Confirm Remote Control action"),
    224                             JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.QUESTION_MESSAGE, null, choices, choices[1]);
    225                 });
    226                 if (tChoice == null) {
    227                     // I have no clue how this would ever happen, but just in case.
    228                     throw new RequestHandlerForbiddenException(MessageFormat.format("RemoteControl: ''{0}'' forbidden due to NPE", myCommand));
     211            final Integer tChoice = GuiHelper.runInEDTAndWaitAndReturn(() -> {
     212                final JLabel label = new JLabel(message);
     213                if (label.getPreferredSize().width > maxWidth) {
     214                    label.setText(message.replaceFirst("<div>", "<div style=\"width:" + maxWidth + "px;\">"));
    229215                }
    230                 choice = tChoice;
    231             } finally {
    232                 MESSAGE_LOCK.unlock();
     216                return JOptionPane.showOptionDialog(MainApplication.getMainFrame(), label, tr("Confirm Remote Control action"),
     217                        JOptionPane.YES_NO_CANCEL_OPTION, JOptionPane.QUESTION_MESSAGE, null, choices, choices[1]);
     218            });
     219            if (tChoice == null) {
     220                // I have no clue how this would ever happen, but just in case.
     221                throw new RequestHandlerForbiddenException(MessageFormat.format("RemoteControl: ''{0}'' forbidden due to NPE", myCommand));
    233222            }
     223            choice = tChoice;
    234224            if (choice != JOptionPane.YES_OPTION && choice != JOptionPane.NO_OPTION) { // Yes/no refer to always/once
    235225                String err = MessageFormat.format("RemoteControl: ''{0}'' forbidden by user''s choice", myCommand);
Note: See TracChangeset for help on using the changeset viewer.