search

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

avoid killing inotifies in all buffers #152

Closed dickmao closed 5 years ago

dickmao commented 5 years ago

generally though inotifies regenerate on their own.

addressing some of alphapapa's concerns in #132

alphapapa commented 5 years ago

Would it not work to simply do (auto-revert-mode -1) in the process buffer? Obviously auto-revert-mode shouldn't be enabled in process buffers, anyway.

alphapapa commented 5 years ago

Upon further inspection of autorevert.el, it seems that non-file buffers should not have auto-revert-mode enabled in the first place. See e.g. variable global-auto-revert-non-file-buffers:

(defcustom global-auto-revert-non-file-buffers nil
  "When nil, Global Auto-Revert Mode operates only on file-visiting buffers.

When non-nil, both file buffers and buffers with a custom
`revert-buffer-function' and a `buffer-stale-function' are
reverted by Global Auto-Revert Mode.  These include the Buffer
List buffer displayed by `buffer-menu', and Dired buffers showing
complete local directories.  The Buffer List buffer reverts every
`auto-revert-interval' seconds; Dired buffers when the file list of
the main directory changes.  Dired buffers do not auto-revert as
a result of changes in subdirectories, or in the contents, size,
modes, etc., of files.  You may still sometimes want to revert
them manually.

Use this option with care since it could lead to excessive auto-reverts.
For more information, see Info node `(emacs)Autorevert'."
  :group 'auto-revert
  :type 'boolean
  :link '(info-link "(emacs)Autorevert"))

I don't see revert-buffer-function anywhere in request.el, so why would it be getting enabled in curl process buffers in the first place? If that's what's happening, it might be a user configuration problem rather than a bug in request or autorevert.

@titaniumbones

dickmao commented 5 years ago

Yes, (auto-revert-mode -1) would work if the default auto-revert-notify-rm-watch wasn't buggy.

alphapapa commented 5 years ago

Yes, (auto-revert-mode -1) would work if the default auto-revert-notify-rm-watch wasn't buggy.

That's the kind of comment that should be found in the code with a TODO to fix it in the future, e.g. something like:

;; The version of this function in Emacs <version-x has this bug: foobar.  
;; We copy this version of the function from Emacs version-y, which fixes it.
;; TODO: When we can stop supporting Emacs <version-y, delete this 
;; code, and instead do (auto-revert-mode -1) in process buffers.

The question remains: why is auto-revert-mode being activated in process buffers in the first place? It seems like that is the real problem. If we find out why that is happening, we might find that it's not even a bug for this package to work around.

dickmao commented 5 years ago

auto-revert-mode probably isn't being activated in process buffers. The code in process.c is blocked on the inotify read descriptor on the buffer where the elisp request code is being C-x C-e'ed

alphapapa commented 5 years ago

Okay, so:

  1. That sounds like a bug in Emacs. Has it been reported to Emacs? If so, we should document the bug number so we can track its progress and adjust the code here accordingly.

  2. Does this bug only happen when code is being evaluated in a file-backed buffer? If so, this workaround seems like a lot of messy, hacky, out-of-scope code to work around an issue that will generally only happen when developing code rather than in the normal course of execution in packages.

    It might be better to simply document that developers should disable auto-revert-mode in Elisp buffers where they are developing code that uses request, and then wait for a fix to be released in Emacs. Some code could also be provided in the documentation that developers could use in their own configs to work around the issue when necessary, rather than having the hacky code in the package affecting all requests for all users.

dickmao commented 5 years ago
  1. If you say so. You can take it up at your leisure.
  2. The bug triggers readily in make test. That is the only thing I really care about. Feel free to ask tkf for Collaborator status, or submit a PR.
alphapapa commented 5 years ago
  1. You don't think it's a bug in Emacs, then?
  2. This package is too important to the Emacs ecosystem to use hacky workarounds that are likely to cause more problems than they solve. :(

@titaniumbones What do you think about all this?

alphapapa commented 5 years ago

Also:

  1. How is make test activating auto-revert-mode with auto-revert-use-notify enabled in a buffer from which Elisp is being evaluated? That makes no sense. Is something wrong with the test suite?
titaniumbones commented 5 years ago

@titaniumbones What do you think about all this?

I guess I would preface my statement by saying that I'm not sure how much my opinion matters. @dickmao took over the work on this package when I basically orphaned it due to some personal circumstances earlier this year, and in any case I was always just a default maintainer who took the job because no one else had time.

That said: I think we could benefit by a better code review practice here. I agree that it is somewhat odd to find misnamed functions in the master branch. It might at the very least be useful to move development to a dedicated branch, say develop, and avoid pushing to master without more consultation. Because of MELPA, every push to master effectively cuts a release, and while the test suite does a lot of work, it has missed a number of bugs in the last 12 months or so.

I also agree with the principle that we should avoid setting global state variables. I have broken my personal Emacs enough times to know that it can be a very fragile apparatus.

So, I'm not really sure what's best here. We have

I unfortunately have a very busy day today and won't be able to look at this again till after dinner time in the ET time zone. I think it would be good if we could all try to move forward as productively as possible -- we all just want to make this package work, because we all need it, right?

titaniumbones commented 5 years ago

also I should say that when I inherited the package, the test suite was pretty opaque to me. I can't remember if the work that @dickmao has done also included substantive reworking of the tests, but certainly it seems possible that the old test suite could have contained any number of bugs.

dickmao commented 5 years ago

How is make test activating auto-revert-mode with auto-revert-use-notify enabled in a buffer from which Elisp is being evaluated? That makes no sense. Is something wrong with the test suite?

Enabling global-auto-revert-mode in the tests triggers the bug reported by louietan in #132. That is all I really care about.

It sounds like you are more concerned about these workarounds than I, so I suggest you do something about it. As de facto maintainer, I can't guarantee I'll accept your PR as-is, but if you somehow get Collaborator status, then you'll have free reign.

One reason why I deleted most of the posts in #132 is the phenomenon of "walls of text" in github issues making it impossible for future readers to discern meaning. Actual code speaks more productively with less verbiage. Please consider my possible future reticence on this issue as general unconcern, not offense.

alphapapa commented 5 years ago

Enabling global-auto-revert-mode in the tests triggers the bug reported by louietan in #132. That is all I really care about.

Part of being a "good citizen" in the Emacs "ecosystem" is reporting bugs upstream so they can be fixed properly, in the right place. It sounds like you did some valuable work digging into process.c, and it's a shame to let that investigation to go waste by resorting to a "gross" workaround and not reporting it upstream. By "gross" I refer to the definition used by e.g. Eli Zaretskii on emacs-devel:

"Gross" means that it solves the problem not where it is caused, and thus makes the maintenance harder by spreading information far from where it should be.

It sounds like you are more concerned about these workarounds than I, so I suggest you do something about it. As de facto maintainer, I can't guarantee I'll accept your PR as-is, but if you somehow get Collaborator status, then you'll have free reign.

If you're going to be a maintainer of an important library like this, I think you should be concerned about fixing these bugs properly, in the right place, and contributing fixes and reports upstream.

I am quite busy with my own Elisp packages, so I'm not interested in spending time submitting PRs which are in conflict with an existing maintainer's work.

I did offer to help maintain this project (as part of a team, not solo) last year. I don't know why my offer was ignored at the time.

My offer still stands, but I would want some clarification of common goals and standards so that our practices would not immediately come into conflict. The maintainers should be on roughly the same page. A Wikipedia-style "edit war" would not be helpful.

One reason why I deleted most of the posts in #132 is the phenomenon of "walls of text" in github issues making it impossible for future readers to discern meaning. Actual code speaks more productively with less verbiage. Please consider my possible future reticence on this issue as general unconcern, not offense.

If this package is going to be maintained by more than one person, communication in human language is going to be necessary. For example, on emacs-devel, the Emacs maintainers do not shy away from thoroughly discussing changes. While those discussions are not always easy, the results speak for themselves: Emacs is quality, long-lived software that users can rely on. I think this library should aim to be the same.

@titaniumbones

I guess I would preface my statement by saying that I'm not sure how much my opinion matters.

Well, the author thought highly enough of you to make you the new maintainer, so it certainly matters! Life happens, and people have to attend to it, but you're still here and you're still you.

That said: I think we could benefit by a better code review practice here. I agree that it is somewhat odd to find misnamed functions in the master branch. It might at the very least be useful to move development to a dedicated branch, say develop, and avoid pushing to master without more consultation. Because of MELPA, every push to master effectively cuts a release, and while the test suite does a lot of work, it has missed a number of bugs in the last 12 months or so.

Agreed.

So, I'm not really sure what's best here. We have...

Agreed.

I unfortunately have a very busy day today and won't be able to look at this again till after dinner time in the ET time zone. I think it would be good if we could all try to move forward as productively as possible -- we all just want to make this package work, because we all need it, right?

Agreed.

Thanks.

titaniumbones commented 5 years ago

I made and self-assigned two reporting/docs issues from points above; tagged each of you in one of them. The changes in this PR should at least be a start towards reducing the invasiveness of this package vis-a-vis the rest of Emacs. @alphapapa are there in your view outstanding technical tasks that can be delineated through bugs or laid out here in the comments?

(proper response sometime after class & dinner)