Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
furkanusta opened this issue Mar 18, 2022 · 1 comment
Open

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

furkanusta opened this issue Mar 18, 2022 · 1 comment

Comments

@furkanusta
Copy link

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
Copy link
Collaborator

dickmao commented Mar 18, 2022

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.

@dickmao dickmao changed the title Passing data with a temporary file instead of sending to process Sending should not rely on UNIX pipes -- it's slow. Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants