Skip to content

Commit

Permalink
core: add a basic timeout of 5s to fetch elpa archive
Browse files Browse the repository at this point in the history 
New file core-emacs-ext.el
This is a basic monkey-patch solution but it will do the job for now.
The timeout amount is not configurable for now.

Tested on 24.5 and 24.3.1

fixes #3284
  • Loading branch information
@syl20bnr
syl20bnr committed Nov 30, 2015
1 parent 648a4e9 commit 4d87ea6
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 0 deletions.
49 changes: 49 additions & 0 deletions core/core-emacs-ext.el
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
;;; core-emacs-ext.el --- Spacemacs Core File
;;
;; Copyright (c) 2012-2014 Sylvain Benner
;; Copyright (c) 2014-2015 Sylvain Benner & Contributors
;;
;; Author: Sylvain Benner <sylvain.benner@gmail.com>
;; URL: https://github.com/syl20bnr/spacemacs
;;
;; This file is not part of GNU Emacs.
;;
;;; License: GPLv3
(require 'core-spacemacs-buffer)

;; for some reason with-eval-after-load does not work here in 24.3
;; maybe the backport is incorrect!
(eval-after-load 'package
'(progn
(defun package-refresh-contents ()
"Download the ELPA archive description if needed.
This informs Emacs about the latest versions of all packages, and
makes them available for download.
This redefinition adds a timeout of 5 seconds to contact each archive."
(interactive)
;; the first part is not available before Emacs 24.4 so we just ignore
;; it to be safe.
(unless (version< emacs-version "24.4")
;; FIXME: Do it asynchronously.
(unless (file-exists-p package-user-dir)
(make-directory package-user-dir t))
(let ((default-keyring (expand-file-name "package-keyring.gpg"
data-directory)))
(when (and package-check-signature (file-exists-p default-keyring))
(condition-case-unless-debug error
(progn
(epg-check-configuration (epg-configuration))
(package-import-keyring default-keyring))
(error (message "Cannot import default keyring: %S" (cdr error)))))))
(dolist (archive package-archives)
(condition-case-unless-debug nil
(with-timeout (5 (spacemacs-buffer/warning
"Cannot contact archive %s (reason: timeout)"
(cdr archive)))
(package--download-one-archive archive "archive-contents"))
(error (message "Failed to download `%s' archive."
(car archive)))))
(package-read-all-archive-contents))))

(provide 'core-emacs-ext)
1 change: 1 addition & 0 deletions core/core-spacemacs.el
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
(require 'core-toggle)
(require 'core-micro-state)
(require 'core-use-package-ext)
(require 'core-emacs-ext)

(defgroup spacemacs nil
"Spacemacs customizations."
Expand Down

22 comments on commit 4d87ea6

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 😠. This has broken M-x list-packages for me on Emacs 25.1…

Can please refrain from advising essential core functions? That's a horrible thing to do, and a darn stupid idea, no matter what issue this could possibly fix.

@bbatsov
Copy link

@bbatsov bbatsov commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaryorn is right. That's a very very bad idea...

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but emacs 25 is not officially supported and the commit message is clear, this is the solution for now tested on 24.3 and 24.5, we all know this is not a good thing but people waiting 120 seconds for their emacs to start is a bigger concern to me. I'll try to make the point upstream when I have some time.

Of course, you are welcome to open an issue or submit a PR, in any case we'll try to fix this quickly :-)

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr Then let them wait. That's a thousand times better than advising the core package function, risking breakage (and not just risking) of the entire thing.

If you're concerned about network connectivity then ping all archives with an aggressive timeout before calling package-refresh-contents. That's way safer than advising.

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the proposal, I'm all for reverting this monkey patching if there is another way!

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr In my opinion you should never have advised this code in the first place. You should never monkey patch core code, no matter what the issue is. It's always going to be worse with advises.

YMMV, obviously, but I can't deny that I've lost trust in Spacemacs to some degree. I had thought that there's a stricter attitude towards code quality, and bringing up Emacs 25 as excuse doesn't help.

I'm sorry that I did not open a PR to fix this issue, but at some point I'd like to expect Spacemacs to just work. After all, I have to get my own work done at some point…

@ReneFroger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr It's considered as a bad idea indeed. Why not revert this change back, and then figure out if there are other alternatives. Like try to determine if the Emacs version 24.3/24.5 is running, then apply this snippet?

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaryorn I understand your point and of course I agree but there is no such thing as all white or all black. I pondered the pros and cons and I decided that for the tested versions of Emacs supported by Spacemacs the likelihood to break something is very, very, very ....... very low, so this solution is a valid temporary one and make a HUGE difference in first perception for people installing Spacemacs while Org elpa directory is down.

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So my point is this: low risk vs. high impact on UX.

Now when you are in development branch of both Spacemacs and Emacs, you cannot expect a perfect experience. If you loose faith in Spacemacs in such context then I cannot do anything for you, especially when the development is very fast and active.

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now if we have a better solution, let's do it, this is a good time for this.

@ReneFroger
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And what about the proposal to determine first if Emacs 24 is running?

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ReneFroger The first improvement would be this, a quick guard for development version of Emacs, but ultimately I would rather see the ping solution proposed by @lunaryorn which seems to be a perfect one.

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr I disagree respectfully. I do think that there is all white and all black in many things, and some things are simply wrong in any case. I think that advising core functions is one of these.

I understand that using development versions of Spacemacs and Emacs is risky, and that's not what I'm concerned about. I expect issues with this combination and I know how to deal with these.

It's the fact that you actually considered an advise as a acceptable solution for whatever issue that concerns me because it reveals an attitude towards quality that's fundamentally different from mine. UX is important, for sure, but in my opinion it's never so important that it should dominate of basic considerations of code quality.

I'm sorry if I'm overreacting, but it's not the first time I've been burned by advises, and I'm getting fed up. I'd go so far as to say that advises—or more precisely, the ability to advise even basic core functions—is one of the worst “features” of Emacs.

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaryorn I agree with you on the fact that advising core function is bad, I apology for this. But let's agree to disagree on the unbreakable rule because pragmatism makes me look at the applied modifications and I cannot help but thinking that this is an educated modification and certainly not a big shot in the dark. But it misses a guard against version below 24.3 and above 24.5 I should have been more accurate on this one especially when it has been tested on 24.3.1 and 24.5.1 only, this is my take away.

No problem with overreacting, I prefer this than no reaction at all and let the code rot.

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr Ok, now that restores my trust a bit ☺️ and I'm sorry for my harsh words 😊 I think you're doing an awesome job with Spacemacs, and despite such little nuisances I'm absolutely enjoying it.

I'm going to delete my tweets about this issue, these were absolutely out of place, and I'm terribly sorry for these, too. 😞

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I added the guard proposed by @ReneFroger but I'll look into the ping solution tonight.

@lunaryorn no problem, we learn to know each other and the project needs people like you capable of raising the voice (as long as it's constructive :-D). I'm sure there are horrors lurking in Spacemacs code base, but rest assure that we'll try to fix them. Now I hope there is not too much of them...

@justbur
Copy link
Contributor

@justbur justbur commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly a stupid question, but where is the 120 sec timeout coming from? I can't seem to find it in the emacs code.

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justbur this is the issue, I was not able to find any timeout configuration anywhere in url library and other places. I'm going to report it upstream (actually should have done that along with the patch...)

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 1, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr I guess that's just the default timeout of connect in Emacs' sources. At least, make-network-process which appears to be the core primitive for network IO doesn't seem to support a connect timeout parameter. It does support asynchronous connections on some systems apparently, but url does not seem to make use of that either to enforce a timeout.

@hijarian
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr It must be really tough, when you genuinely care about your userbase, make a decision after considering all pros and cons, and then somebody just comes at you and start talking to you like you're some random nobody and do not know what you're doing at all. :( That's physically painful to see. Personally, I fully support this patch if it'll actually prevent the 2 minutes waiting before Spacemacs start (haven't experienced anything like that, though).

@syl20bnr
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunaryorn I implemented a solution inspired by your idea to ping the domain, it is based on request.el. See d822241 and more precisely https://github.com/syl20bnr/spacemacs/blob/develop/core/core-configuration-layer.el#L189-L230
Thank you for the pointer :-)

@swsnr
Copy link
Contributor

@swsnr swsnr commented on 4d87ea6 Dec 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@syl20bnr Thank you 😊

Please sign in to comment.