Make cancellation of uploads work regardless of Push configuration (#11743) (#11760)

- Checking the push configuration outside of session lock threw
  an AssertionError, so the push configuration is not checked anymore.

- The original problem with cancelling Upload was due to a subtle
  ordering issue that depended on the Push configuration.
  In the case of PushMode.AUTOMATIC, a new StreamVariable was
  added by the `Upload` component _before_ the `FileUploadHandler`
  got a chance to remove the old `StreamVariable`. As a result, the
  `FileUploadHandler` actually removed the fresh `StreamVariable`,
  breaking future uploads.

Fixes #11682

6 files changed, 128 insertions, 13 deletions

diff --git a/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java b/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
index 64e0df1b06..bd20bea9ed 100644
--- a/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
+++ b/server/src/main/java/com/vaadin/server/communication/FileUploadHandler.java
@@ -621,11 +621,7 @@ public class FileUploadHandler implements RequestHandler {
             } finally {
                 session.unlock();
             }
-            boolean pushEnabled = UI.getCurrent().getPushConfiguration()
-                    .getPushMode().isEnabled();
-            if (!pushEnabled) {
-                return true;
-            }
+            return true;
 
             // Note, we are not throwing interrupted exception forward as it is
             // not a terminal level error like all other exception.
@@ -704,7 +700,20 @@ public class FileUploadHandler implements RequestHandler {
 
     private void cleanStreamVariable(VaadinSession session, final UI ui,
             final ClientConnector owner, final String variableName) {
-        session.accessSynchronously(() -> ui.getConnectorTracker()
-                .cleanStreamVariable(owner.getConnectorId(), variableName));
+        session.accessSynchronously(() -> {
+            ui.getConnectorTracker().cleanStreamVariable(owner.getConnectorId(),
+                    variableName);
+
+            // in case of automatic push mode, the client connector
+            // could already have refreshed its StreamVariable
+            // in the ConnectorTracker. For instance, the Upload component
+            // adds its stream variable in its paintContent method, which is
+            // called (indirectly) on each session unlock in case of automatic
+            // pushes.
+            // To cover this case, mark the client connector as dirty, so that
+            // the unlock after this runnable refreshes the StreamVariable
+            // again.
+            owner.markAsDirty();
+        });
     }
 }
diff --git a/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUpload.java b/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUpload.java
index 8d95b35373..9b4a201c0f 100644
--- a/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUpload.java
+++ b/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUpload.java
@@ -22,6 +22,15 @@ public class InterruptUpload extends AbstractTestUI {
 
     private Upload sample;
     private UploadInfoWindow uploadInfoWindow;
+    private final boolean pushManually;
+
+    public InterruptUpload() {
+        this(false);
+    }
+
+    public InterruptUpload(boolean pushManually) {
+        this.pushManually = pushManually;
+    }
 
     @Override
     protected void setup(VaadinRequest request) {
@@ -39,13 +48,17 @@ public class InterruptUpload extends AbstractTestUI {
                 UI.getCurrent().addWindow(uploadInfoWindow);
             }
             uploadInfoWindow.setClosable(false);
+            pushIfManual();
+        });
+        sample.addFinishedListener(event -> {
+            uploadInfoWindow.setClosable(true);
+            pushIfManual();
         });
-        sample.addFinishedListener(event -> uploadInfoWindow.setClosable(true));
 
-        addComponent(sample);
+        addComponents(new Label(getDescription()), sample);
     }
 
-    private static class UploadInfoWindow extends Window
+    private class UploadInfoWindow extends Window
             implements Upload.StartedListener, Upload.ProgressListener,
             Upload.FailedListener, Upload.SucceededListener,
             Upload.FinishedListener {
@@ -114,6 +127,7 @@ public class InterruptUpload extends AbstractTestUI {
             textualProgress.setVisible(false);
             cancelButton.setVisible(false);
             UI.getCurrent().setPollInterval(-1);
+            pushIfManual();
         }
 
         @Override
@@ -128,6 +142,7 @@ public class InterruptUpload extends AbstractTestUI {
             fileName.setValue(event.getFilename());
 
             cancelButton.setVisible(true);
+            pushIfManual();
         }
 
         @Override
@@ -138,11 +153,13 @@ public class InterruptUpload extends AbstractTestUI {
             textualProgress.setValue(
                     "Processed " + readBytes + " bytes of " + contentLength);
             result.setValue(counter.getLineBreakCount() + " (counting...)");
+            pushIfManual();
         }
 
         @Override
         public void uploadSucceeded(final SucceededEvent event) {
             result.setValue(counter.getLineBreakCount() + " (total)");
+            pushIfManual();
         }
 
         @Override
@@ -150,6 +167,13 @@ public class InterruptUpload extends AbstractTestUI {
             result.setValue(
                     counter.getLineBreakCount() + " (counting interrupted at "
                             + Math.round(100 * progressBar.getValue()) + "%)");
+            pushIfManual();
+        }
+    }
+
+    private void pushIfManual() {
+        if (pushManually) {
+            push();
         }
     }
 
diff --git a/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUploadWithManualPush.java b/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUploadWithManualPush.java
new file mode 100644
index 0000000000..0f60753fc5
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/components/upload/InterruptUploadWithManualPush.java
@@ -0,0 +1,23 @@
+package com.vaadin.tests.components.upload;
+
+import com.vaadin.annotations.Push;
+import com.vaadin.shared.communication.PushMode;
+
+@Push(PushMode.MANUAL)
+public class InterruptUploadWithManualPush extends InterruptUpload {
+
+    public InterruptUploadWithManualPush() {
+        super(true);
+    }
+
+    @Override
+    protected Integer getTicketNumber() {
+        return 11616;
+    }
+
+    @Override
+    public String getDescription() {
+        return "Interrupting an upload with @Push shouldn't prevent uploading that same file immediately afterwards.";
+    }
+
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUpload.java b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUpload.java
index 53264a760d..6189cf5eaf 100644
--- a/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUpload.java
+++ b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUpload.java
@@ -4,15 +4,18 @@ import java.io.IOException;
 import java.io.OutputStream;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import com.vaadin.server.StreamVariable;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.shared.ui.dnd.FileParameters;
 import com.vaadin.shared.ui.grid.DropMode;
 import com.vaadin.tests.components.AbstractTestUIWithLog;
+import com.vaadin.ui.Button;
 import com.vaadin.ui.Grid;
-import com.vaadin.ui.Layout;
 import com.vaadin.ui.Notification;
+import com.vaadin.ui.ProgressBar;
+import com.vaadin.ui.UI;
 import com.vaadin.ui.VerticalLayout;
 import com.vaadin.ui.components.grid.GridDropTarget;
 import com.vaadin.ui.dnd.FileDropTarget;
@@ -21,6 +24,16 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
 
     private static final int FILE_SIZE_LIMIT = 1024 * 1024 * 5; // 5 MB
 
+    private final boolean pushManually;
+
+    public Html5FileDragAndDropUpload() {
+        this(false);
+    }
+
+    public Html5FileDragAndDropUpload(boolean pushManually) {
+        this.pushManually = pushManually;
+    }
+
     @Override
     protected void setup(VaadinRequest request) {
 
@@ -31,6 +44,9 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
         grid.addColumn(FileParameters::getMime).setCaption("Mime type");
 
         List<FileParameters> gridItems = new ArrayList<>();
+        AtomicBoolean cancelled = new AtomicBoolean(false);
+
+        ProgressBar progressBar = new ProgressBar(0.0f);
 
         new FileDropTarget<Grid<FileParameters>>(grid,
                 event -> event.getFiles().forEach(html5File -> {
@@ -39,6 +55,7 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
                                 + " is too large (max 5 MB)");
                         return;
                     }
+                    UI.getCurrent().setPollInterval(200);
 
                     html5File.setStreamVariable(new StreamVariable() {
                         @Override
@@ -58,19 +75,27 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
 
                         @Override
                         public void onProgress(StreamingProgressEvent event) {
+                            float progress = (float) event.getBytesReceived()
+                                    / event.getContentLength();
+                            progressBar.setValue(progress);
                             log("Progress, bytesReceived="
                                     + event.getBytesReceived());
+                            pushIfManual();
                         }
 
                         @Override
                         public void streamingStarted(
                                 StreamingStartEvent event) {
+                            cancelled.set(false);
+                            progressBar.setValue(0.0f);
                             log("Stream started, fileName="
                                     + event.getFileName());
+                            pushIfManual();
                         }
 
                         @Override
                         public void streamingFinished(StreamingEndEvent event) {
+                            progressBar.setValue(1.0f);
                             gridItems
                                     .add(new FileParameters(event.getFileName(),
                                             event.getContentLength(),
@@ -79,17 +104,22 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
 
                             log("Stream finished, fileName="
                                     + event.getFileName());
+                            pushIfManual();
+                            UI.getCurrent().setPollInterval(-1);
                         }
 
                         @Override
                         public void streamingFailed(StreamingErrorEvent event) {
+                            progressBar.setValue(0.0f);
                             log("Stream failed, fileName="
                                     + event.getFileName());
+                            pushIfManual();
+                            UI.getCurrent().setPollInterval(-1);
                         }
 
                         @Override
                         public boolean isInterrupted() {
-                            return false;
+                            return cancelled.get();
                         }
                     });
                 }));
@@ -101,11 +131,20 @@ public class Html5FileDragAndDropUpload extends AbstractTestUIWithLog {
             Notification.show(event.getDataTransferText());
         });
 
-        Layout layout = new VerticalLayout(grid);
+        Button cancelButton = new Button("Cancel",
+                click -> cancelled.set(true));
+        VerticalLayout layout = new VerticalLayout(grid, cancelButton,
+                progressBar);
 
         addComponent(layout);
     }
 
+    private void pushIfManual() {
+        if (pushManually) {
+            push();
+        }
+    }
+
     @Override
     protected String getTestDescription() {
         return "Drop files onto the Grid to upload them or text";
diff --git a/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithManualPush.java b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithManualPush.java
new file mode 100644
index 0000000000..22b63aacb2
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithManualPush.java
@@ -0,0 +1,12 @@
+package com.vaadin.tests.dnd;
+
+import com.vaadin.annotations.Push;
+import com.vaadin.shared.communication.PushMode;
+
+@Push(PushMode.MANUAL)
+public class Html5FileDragAndDropUploadWithManualPush
+        extends Html5FileDragAndDropUpload {
+    public Html5FileDragAndDropUploadWithManualPush() {
+        super(true);
+    }
+}
diff --git a/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithPush.java b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithPush.java
new file mode 100644
index 0000000000..9425a05c5b
--- /dev/null
+++ b/uitest/src/main/java/com/vaadin/tests/dnd/Html5FileDragAndDropUploadWithPush.java
@@ -0,0 +1,8 @@
+package com.vaadin.tests.dnd;
+
+import com.vaadin.annotations.Push;
+
+@Push
+public class Html5FileDragAndDropUploadWithPush
+        extends Html5FileDragAndDropUpload {
+}