Merge pull request #62 from NfNitLoop/release-v0.5.1

Release v0.5.1

Author: NfNitLoop

CHANGELOG.md

@@ -1,6 +1,25 @@
 Changelog
 =========
 
+Version 0.5.1
+=============
+
+Released: July 24, 2021  
+<https://github.com/NfNitLoop/feoblog/releases/tag/v0.5.1>
+
+
+Improvements
+------------
+
+ * Better handling of attachment uploads.  
+   For cases where it already had a copy of an attachment on the server,
+   FeoBlog would close the HTTP connection early with a 202 status. This
+   didn't work well in Chrome or with Deno. Now we'll try to be a bit
+   more friendly to the client.
+
+   As an user, you *may* notice slightly faster uploads of duplicate files.
+   Though I don't expect that that's a common case.
+
 Version 0.5.0
 =============
 

src/server/attachments.rs

@@ -64,10 +64,16 @@ pub(crate) async fn put_file(
     let backend = data.backend_factory.open()?;
 
     let metadata = backend.get_attachment_meta(&user_id, &signature, &file_name)?;
+    // Drop our pooled connection while we wait for bytes, which could be slow:
+    drop(backend);
 
     let metadata = match metadata {
         Some(d) => d,
         None => {
+            // note: NO drain() here.
+            // see: https://stackoverflow.com/questions/14250991/is-it-acceptable-for-a-server-to-send-a-http-response-before-the-entire-request
+            // regarding HTTP errros. This is an error condition anyway.
+
             // If we don't yet have the metadata for a file (provided in its Item), then you can't upload yet.
             return Ok(
                 HttpResponse::Forbidden()
@@ -78,6 +84,7 @@ pub(crate) async fn put_file(
     };
 
     if metadata.exists {
+        drain(body).await;
         return Ok(
             HttpResponse::Accepted()
             .content_type(PLAINTEXT)
@@ -119,8 +126,6 @@ pub(crate) async fn put_file(
 
     // Collect the file bytes into a temp file so that we're not using the backend while we wait for the upload:
 
-    // Drop our pooled connection while we wait for bytes, which could be slow:
-    drop(backend);
 
     let file = tempfile().context("Error opening temp file")?;
 
@@ -185,6 +190,22 @@ pub(crate) async fn put_file(
     );
 }
 
+
+/// If you don't wait to read all the Payload bytes, Actix-Web may close
+/// the connection before the client has sent them all. Then the client
+/// may behave poorly.
+/// 
+/// 1. Chrome has a long pause.
+/// 2. Deno fetch() dies on the next request.  
+///    See: https://github.com/denoland/deno/issues/11513
+/// 
+/// SO, here we read the whole payload, even though we don't care what
+/// they sent. This seems ripe for DoS but it just seems to be the way
+/// browsers and HTTP clients work? 😢
+async fn drain(mut payload: Payload) {
+    while payload.next().await.is_some() {}
+}
+
 pub(crate) async fn head_file(
     data: Data<AppData>,
     Path((user_id, signature, file_name)): Path<(UserID, Signature, String)>,

web-client/components/pages/SyncPage.svelte

@@ -332,8 +332,8 @@ async function syncAttachment({userID, signature, fileName, to, fromServers, tra
         try {
             await tracker.runSubtask(`Uploading to ${to.url}`, async (tracker) => {
                 let blob = new Blob([buffer!])
-                let response = await to.putAttachment(userID, signature, fileName, blob)
-                tracker.log(`Success. ${response.status}: ${response.statusText}`)
+                await to.putAttachment(userID, signature, fileName, blob)
+                tracker.log(`Success.`)
             })
         } catch (_ignored) {
             // The subtask will have recorded the error.

web-client/ts/client.ts

@@ -4,6 +4,10 @@ import * as nacl from "./naclWorker/nacl"
 import { bytesToHex, ConsoleLogger, Logger, prefetch } from "./common";
 import { tick } from "svelte";
 
+// Before sending files larger than this, we should check whether they exist:
+const SMALL_FILE_THRESHOLD = 1024 * 128
+
+
 // Encapsulates communication with the server(s).
 export class Client {
 
@@ -96,7 +100,13 @@ export class Client {
         return response
     }
 
-    async putAttachment(userID: UserID, signature: Signature, fileName: string, blob: Blob): Promise<Response> {
+    async putAttachment(userID: UserID, signature: Signature, fileName: string, blob: Blob): Promise<void> {
+        // If the file is already in the content store, we can save some bandwidth/time:
+        if (blob.size > SMALL_FILE_THRESHOLD) {
+            const {exists} = await this.headAttachment(userID, signature, fileName)
+            if (exists) return
+        }
+
         let url = this.attachmentURL(userID, signature, fileName)
         let response: Response
         try {
@@ -109,11 +119,11 @@ export class Client {
             }
 
         } catch (e) {
-            console.error("PUT exception:", e)
+            const {exists} = await this.headAttachment(userID, signature, fileName)
+            if (exists) return // Someone beat us to the upload, everything's OK.
+            // else:
             throw e
         }
-
-        return response
     }
 
     private attachmentURL(userID: UserID, signature: Signature, fileName: string) {
@@ -339,7 +349,7 @@ export type AttachmentMeta = {
 
 export type GetItemOptions = {
     // When syncing items from one server to another, the receiving server MUST 
-    // perform the verificiation, so verifying in the client is redundant and slow.
+    // perform the verification, so verifying in the client is redundant and slow.
     // Set this flag to skip it.
     skipSignatureCheck?: boolean
 }