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

Inexplicable hang on Emacs 30 snapshot #247

Closed
alphapapa opened this issue Jun 27, 2024 · 6 comments · Fixed by #249
Closed

Inexplicable hang on Emacs 30 snapshot #247

alphapapa opened this issue Jun 27, 2024 · 6 comments · Fixed by #249

Comments

@alphapapa
Copy link
Contributor

Hi,

I'm seeing that my tests for org-ql with Buttercup are hanging exclusively on Emacs snapshot (currently an Emacs 30 pre-release version). I don't know if this indicates a bug in Buttercup, or an incompatible change in Emacs 30, but I hope that we can find the problem and solve it before Emacs 30.1 is released. These hangs are happening with the latest version of Buttercup installed automatically from MELPA during the CI run.

Perhaps notably, perhaps related, Guix has disabled tests for org-ql due to what seems to be a similar problem: https://git.savannah.gnu.org/cgit/guix.git/commit/gnu/packages/emacs-xyz.scm?id=3add97c7761e6c58a1d7405f417a49dda5f0a742

* gnu/packages/emacs-xyz.scm (emacs-org-ql): Update to 0.8.6.
[arguments]<#:tests?>: Disable tests, which freeze starting with recent
Buttercup releases.

I can't reproduce the problem locally, being on a different Emacs version. But besides that, earlier today while debugging similar issues, I had similar hangs during a Buttercup run on other Emacs/Org/Buttercup versions, and I don't know how to "look inside" a Buttercup run to find out what's hanging.

Any suggestions would be appreciated. Thanks.

@snogge
Copy link
Collaborator

snogge commented Jun 27, 2024

I was able to reproduce. It not only hangs, it also eats a lot of memory.
This is probably the stacktrace generation and the fact that buttercup does not use print-circle t.
The reason for the failing tests is a change in read in Emacs 30. This is probably unintended as I cannot see any mention of this changed behavior in NEWS. It also looks like it should still work. I have not filed any Emacs bug, and I have not searched the bug database.

@alphapapa
Copy link
Contributor Author

alphapapa commented Jun 28, 2024

Wow, that must have taken a while to research. Thanks for digging in to that. Have you found the commit in emacs.git that makes the incompatible change to read?

@alphapapa
Copy link
Contributor Author

alphapapa commented Jun 28, 2024

After looking at your comment on alphapapa/org-ql#442, I remembered something I saw recently: I wonder if the problem could be this: https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=f2bccae22bd47a2e7e0937b78ea06131711b935a (NEWS entry)

@snogge
Copy link
Collaborator

snogge commented Jun 29, 2024

I just tried to isolate the hanging test, and once I got the test suite to actually exit I noticed some failing tests.
Interestingly, disabling all but the hanging test also resolved the infloop. I'm guessing this means you have some dependency between your tests.
Anyway, once I found the first failing test it was fairly easy to find the naive solution of just taking the car before calling read. But there was probably an element of luck involved.

The Emacs change you point out could be responsible for this breakage. I don't understand the C parts of Emacs very well. The patch does not touch the obvious pieces of lread.c but there are probably interactions that I do not see.

Looking at my comments on alphapapa/org-ql#442 I probably drew the wrong conclusion as to why the car was needed. Still not sure I understand it.

@alphapapa
Copy link
Contributor Author

@snogge It seems that, once the accidental misuse of read is fixed, (e.g. see https://github.com/alphapapa/org-ql/tree/wip/test-fix-read-issue where I made the changes in a test branch--I will credit you when merging for "real"), all of the tests pass. (Apparently I never noticed that I was calling read with a list instead of a string; I don't know why it ever worked. The comment there mentions that url returns an "improper alist", but I must never have noticed that it needed a further step to access the value properly.)

On the Buttercup side, are there any improvements that could be made to prevent its apparently hanging in a situation like this? Or that would make it easier for an end-user like me to debug?

Thank you for digging into this problem. I regret that it turned out to be caused by a minor bug in org-ql, but hopefully this discovery will benefit Buttercup as well.

@snogge
Copy link
Collaborator

snogge commented Jul 18, 2024

@alphapapa
As far as I can tell, the out-of-memory error is because of the `crop' style of backtrace used by default. Both the stacktrace line format operation and crop are done as string operations and can consume a lot of memory with deeply nested structures. And that type of structure is common in org.

I've done some digging around in the buttercup backtrace code and found lots of room for improvement.
This did not surprise me, it's always been a series of kludges.
I've done some improvements to the code - not all of which are merged yet - which not only seem to avoid the out-of-memory bug triggered by org-ql but also improves performance a lot and removes most of the buttercup internal code from the backtrace.
Still some work to be done though.

alphapapa added a commit to alphapapa/org-ql that referenced this issue Sep 5, 2024
This is needed due to changes in Emacs 30, but this code was always
mistaken, and it just happened to work.

See <jorgenschaefer/emacs-buttercup#247>.

Fixes #461.

Suggested-by: Ola Nilsson <[email protected]>
alphapapa added a commit to alphapapa/org-ql that referenced this issue Sep 5, 2024
This is needed due to changes in Emacs 30, but this code was always
mistaken, and it just happened to work.

See <jorgenschaefer/emacs-buttercup#247>.

Fixes #441.

Suggested-by: Ola Nilsson <[email protected]>
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

Successfully merging a pull request may close this issue.

2 participants