tkf / emacs-request

Request.el -- Easy HTTP request for Emacs Lisp
http://tkf.github.com/emacs-request/
GNU General Public License v3.0
629 stars 93 forks source link

Sending should not rely on UNIX pipes -- it's slow. #207

Open furkanusta opened 2 years ago

furkanusta commented 2 years ago

Sending a tar file (~25MB) in the request body took around ~7sec, while it was near instant in the curl process. Further investigation revealed that the bottleneck is process-send-region

Creating a temporary buffer and passing the location of that to curl reduced the execution time drastically. What I am doing looks like this.

@@ -888,7 +888,11 @@
             collect (format "%s: %s" k v))
    (list "--url" url)
    (when data
-     (split-string "--data-binary @-"))
+     (let ((file (make-temp-file "request-")))
+       (with-temp-file file
+         (set-buffer-file-coding-system 'raw-text)
+         (insert data))
+       (list "--data-binary" (concat "@" file))))
    (cl-loop with stdin-p = data
             for (name . item) in files
             collect "--form"
@@ -988,7 +992,7 @@
     (set-process-coding-system proc 'no-conversion 'no-conversion)
     (set-process-query-on-exit-flag proc nil)
     (process-send-string proc stdin-config)
-    (when (or data file-buffer file-data)
+    (when (or file-buffer file-data)
       ;; We dynamic-let the global `buffer-file-coding-system' to `no-conversion'
       ;; in case the user-configured `encoding' doesn't fly.
       ;; If we do not dynamic-let the global, `select-safe-coding-system' would

Would you like me to create a pull request with a similar change applied to files parameter as well? I understand if you do not want to modify an already working code.

dickmao commented 2 years ago

Damn, that is brutal. Thanks for reporting.

Alas, the now-in-retrospect insanity of relying on UNIX pipes to promptly transmit bytes to the curl process originates from #202 . It's hard to imagine any emacs user having data worth stealing, but people tend to get sanctimonious about security holes, even if those obvious holes are rarely the ones black hats exploit (it's never the devil you know that ends up killing you).

I've recorded the slowdown in a test in c769cf3. Since most users use request.el to pull, and the minority that pushes doesn't push 25MB, I'm content to let this performance bug percolate.