* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
@ 2016-07-14 14:03 Chris Feng
2017-11-19 9:39 ` Chris Feng
0 siblings, 1 reply; 15+ messages in thread
From: Chris Feng @ 2016-07-14 14:03 UTC (permalink / raw)
To: 23980
I encountered a bug in the command loop that when I put an event of the
form (t . EVENT) into `unread-command-events', there is a chance the
event won't get recognized with the message "<t> is undefined".
I did a bit research and found that in keyboard.c `read_char' calls
`read_decoded_event_from_main_queue' (which in turn calls
`read_event_from_main_queue' and `kbd_buffer_get_event') but does not
check if the returned event is from `Vunread_command_events'.
The problem in `read_char' can be demonstrated by the following form:
(progn
(run-with-timer 1 nil (lambda () (push '(t . 1) unread-command-events)))
(read-event nil nil 2))
=> '(t . 1)
There is actually another related issue, that for an event returned by
`read_decoded_event_from_main_queue' other than the form (t . EVENT), we
can't tell whether it's from `Vunread_command_events' or not.
If it is, then we should not put it into `this_command_keys'.
Any ideas on how to fix these?
^ permalink raw reply [flat|nested] 15+ messages in thread* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2016-07-14 14:03 bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled Chris Feng
@ 2017-11-19 9:39 ` Chris Feng
2018-02-20 15:35 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Chris Feng @ 2017-11-19 9:39 UTC (permalink / raw)
To: 23980-done
[-- Attachment #1: Type: text/plain, Size: 33 bytes --]
The bug was fixed by 1f3f4b1296.
[-- Attachment #2: Type: text/html, Size: 55 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2017-11-19 9:39 ` Chris Feng
@ 2018-02-20 15:35 ` Dmitry Gutov
2018-02-20 18:37 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2018-02-20 15:35 UTC (permalink / raw)
To: 23980, Eli Zaretskii
[-- Attachment #1: Type: text/plain, Size: 733 bytes --]
On 11/19/17 11:39 AM, Chris Feng wrote:
> The bug was fixed by 1f3f4b1296.
Eli, any change to have this backported to emacs-26?
This reason I'm asking is, this fixed a rather nasty bug resulting from
the interaction between the newest company-mode (0.9.5) and
flyspell-mode, both of which use sit-for. This particular combination
results in a timer being run inside a sit-for, which also calls sit-for,
which in emacs-26 ends up putting e.g. (t t . 45) into
unread-command-events.
Here's how to reproduce:
- Visit the attached file.
- M-x eval-buffer.
- Start typing.
- See "<t> is undefined" in the minibuffer for every other keypress.
Originally reported here:
https://github.com/company-mode/company-mode/issues/760
[-- Attachment #2: sit-for-bug.el --]
[-- Type: text/x-emacs-lisp, Size: 427 bytes --]
(defvar foo-timer nil)
(defun foo-pre-command ()
(when foo-timer
(cancel-timer foo-timer)
(setq foo-timer nil)))
(defun foo-post-command ()
(run-with-timer 0.01 nil #'foo-idle))
(defun foo-idle ()
(sit-for 5))
(add-hook 'pre-command-hook 'foo-pre-command)
(add-hook 'post-command-hook 'foo-post-command)
(flyspell-mode)
;; This causes unread-command-events to sometimes contain
;; e.g. ((t t . 45))
mmmmmm
^ permalink raw reply [flat|nested] 15+ messages in thread* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-20 15:35 ` Dmitry Gutov
@ 2018-02-20 18:37 ` Eli Zaretskii
2018-02-20 21:16 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2018-02-20 18:37 UTC (permalink / raw)
To: Dmitry Gutov, Chris Feng; +Cc: 23980
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 20 Feb 2018 17:35:15 +0200
>
> On 11/19/17 11:39 AM, Chris Feng wrote:
> > The bug was fixed by 1f3f4b1296.
>
> Eli, any change to have this backported to emacs-26?
I'd need someone (Chris?) to tell which situations cause (t SOMETHING)
be put into unread-command-events. If these situations are rare
enough, maybe we can risk it.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-20 18:37 ` Eli Zaretskii
@ 2018-02-20 21:16 ` Dmitry Gutov
2018-02-21 4:22 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2018-02-20 21:16 UTC (permalink / raw)
To: Eli Zaretskii, Chris Feng; +Cc: 23980
On 2/20/18 8:37 PM, Eli Zaretskii wrote:
> I'd need someone (Chris?) to tell which situations cause (t SOMETHING)
> be put into unread-command-events. If these situations are rare
> enough, maybe we can risk it.
At least one of the places that do that is sit-for. See the long comment
at the end of its definition (I don't really understand its last
sentence, FWIW).
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-20 21:16 ` Dmitry Gutov
@ 2018-02-21 4:22 ` Eli Zaretskii
2018-02-21 7:40 ` Chris Feng
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2018-02-21 4:22 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 23980, chris.w.feng
> Cc: 23980@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 20 Feb 2018 23:16:47 +0200
>
> On 2/20/18 8:37 PM, Eli Zaretskii wrote:
>
> > I'd need someone (Chris?) to tell which situations cause (t SOMETHING)
> > be put into unread-command-events. If these situations are rare
> > enough, maybe we can risk it.
>
> At least one of the places that do that is sit-for. See the long comment
> at the end of its definition (I don't really understand its last
> sentence, FWIW).
But then how come you have (t t . 45) there?
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 4:22 ` Eli Zaretskii
@ 2018-02-21 7:40 ` Chris Feng
2018-02-21 10:47 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Chris Feng @ 2018-02-21 7:40 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 23980, Dmitry Gutov
Eli Zaretskii <eliz@gnu.org> writes:
>> On 2/20/18 8:37 PM, Eli Zaretskii wrote:
>>
>> > I'd need someone (Chris?) to tell which situations cause (t SOMETHING)
>> > be put into unread-command-events. If these situations are rare
>> > enough, maybe we can risk it.
>>
>> At least one of the places that do that is sit-for. See the long comment
>> at the end of its definition (I don't really understand its last
>> sentence, FWIW).
It seems to be the only use case in Emacs's source tree, but we can't
tell how many packages do this trick in the wild nor can we predict
that.
> But then how come you have (t t . 45) there?
Probably shortly after (t . 45) is unread another call to `sit-for'
happens to read it out directly from `unread-command-events', and the
event is further unread as (cons t '(t . 45)). Perhaps we may even get
something like (t t t . 45) if lucky enough.
PS. Many users will stick with the pre-26 releases in the next upcoming
years so backporting this fix does not necessarily resolve the problem
for company-mode. At least a workaround has to be come up with to deal
with the situations where this fix is absent.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 7:40 ` Chris Feng
@ 2018-02-21 10:47 ` Dmitry Gutov
2018-02-21 12:23 ` Dmitry Gutov
2018-02-21 17:48 ` Eli Zaretskii
0 siblings, 2 replies; 15+ messages in thread
From: Dmitry Gutov @ 2018-02-21 10:47 UTC (permalink / raw)
To: Chris Feng, Eli Zaretskii; +Cc: 23980
On 2/21/18 9:40 AM, Chris Feng wrote:
> It seems to be the only use case in Emacs's source tree, but we can't
> tell how many packages do this trick in the wild nor can we predict
> that.
They only do that with one goal, though (so we are dealing with a protocol).
>> But then how come you have (t t . 45) there?
>
> Probably shortly after (t . 45) is unread another call to `sit-for'
> happens to read it out directly from `unread-command-events', and the
> event is further unread as (cons t '(t . 45)).
More or less, this. Except we don't get this effect from two (or more)
consecutive calls to sit-for.
What triggers it, is effectively recursive sit-for calls, when the
second sit-for is called inside a timer, which are allowed to run inside
the first sit-for.
> PS. Many users will stick with the pre-26 releases in the next upcoming
> years so backporting this fix does not necessarily resolve the problem
> for company-mode. At least a workaround has to be come up with to deal
> with the situations where this fix is absent.
I've been trying to. So far, the options are:
- Advise users to lower flyspell-delay to (* 0.9 company-idle-delay).
That actually leads to freezes, which I should file as a separate bug
report.
- Ask people to disable flyspell-mode :)
- Use sleep-for instead of sit-for in the relevant place, forgoing the
significant latency improvement that comes with the latter. Maybe
predicated on Emacs version, and flyspell-mode being enabled.
So far the last option is winning, but I'd be much more content to use
it if the Emacs 26 are not going to need it.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 10:47 ` Dmitry Gutov
@ 2018-02-21 12:23 ` Dmitry Gutov
2018-02-21 17:48 ` Eli Zaretskii
1 sibling, 0 replies; 15+ messages in thread
From: Dmitry Gutov @ 2018-02-21 12:23 UTC (permalink / raw)
To: Chris Feng, Eli Zaretskii; +Cc: 23980
On 2/21/18 12:47 PM, Dmitry Gutov wrote:
> - Use sleep-for instead of sit-for in the relevant place, forgoing the
> significant latency improvement that comes with the latter<...>
Or maybe significant-ish. To the tune of 30ms.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 10:47 ` Dmitry Gutov
2018-02-21 12:23 ` Dmitry Gutov
@ 2018-02-21 17:48 ` Eli Zaretskii
2018-02-21 21:45 ` Dmitry Gutov
1 sibling, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2018-02-21 17:48 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 23980, chris.w.feng
> Cc: 23980@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 21 Feb 2018 12:47:20 +0200
>
> So far the last option is winning, but I'd be much more content to use
> it if the Emacs 26 are not going to need it.
I'm _really_ uneasy about putting this on the release branch. sit-for
is notorious for subtle problems that take a lot of time to be
exposed.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 17:48 ` Eli Zaretskii
@ 2018-02-21 21:45 ` Dmitry Gutov
2018-02-22 7:39 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2018-02-21 21:45 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 23980, chris.w.feng
On 2/21/18 7:48 PM, Eli Zaretskii wrote:
>> Cc: 23980@debbugs.gnu.org
>> From: Dmitry Gutov <dgutov@yandex.ru>
>> Date: Wed, 21 Feb 2018 12:47:20 +0200
>>
>> So far the last option is winning, but I'd be much more content to use
>> it if the Emacs 26 are not going to need it.
>
> I'm _really_ uneasy about putting this on the release branch. sit-for
> is notorious for subtle problems that take a lot of time to be
> exposed.
Well, ok. I can live with that.
Maybe consider it for inclusion if we end up having more pretests? This
patch has been in master for a few months, and it's two lines long, so
it must be *somewhat* safe.
I've pushed the workaround (last option) to company-mode in the meantime.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-21 21:45 ` Dmitry Gutov
@ 2018-02-22 7:39 ` Eli Zaretskii
2018-12-19 13:54 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2018-02-22 7:39 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 23980, chris.w.feng
> Cc: chris.w.feng@gmail.com, 23980@debbugs.gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 21 Feb 2018 23:45:30 +0200
>
> > I'm _really_ uneasy about putting this on the release branch. sit-for
> > is notorious for subtle problems that take a lot of time to be
> > exposed.
>
> Well, ok. I can live with that.
>
> Maybe consider it for inclusion if we end up having more pretests? This
> patch has been in master for a few months, and it's two lines long, so
> it must be *somewhat* safe.
Yes, a good idea.
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-02-22 7:39 ` Eli Zaretskii
@ 2018-12-19 13:54 ` Dmitry Gutov
2018-12-19 15:27 ` Eli Zaretskii
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2018-12-19 13:54 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 23980, chris.w.feng
Hi Eli,
On 22.02.2018 9:39, Eli Zaretskii wrote:
>> Maybe consider it for inclusion if we end up having more pretests? This
>> patch has been in master for a few months, and it's two lines long, so
>> it must be *somewhat* safe.
>
> Yes, a good idea.
Any chance we can backport it now? I have hard-to-reproduce bug report
from users of third-party code (company-mode plus elpy) which, I am
told, is definitely fixed by applying this patch to emacs-26:
https://github.com/jorgenschaefer/elpy/issues/1396#issuecomment-448142207
^ permalink raw reply [flat|nested] 15+ messages in thread
* bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled
2018-12-19 13:54 ` Dmitry Gutov
@ 2018-12-19 15:27 ` Eli Zaretskii
2018-12-19 16:07 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Eli Zaretskii @ 2018-12-19 15:27 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: 23980, chris.w.feng
> Cc: 23980@debbugs.gnu.org, chris.w.feng@gmail.com
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Wed, 19 Dec 2018 15:54:31 +0200
>
> On 22.02.2018 9:39, Eli Zaretskii wrote:
>
> >> Maybe consider it for inclusion if we end up having more pretests? This
> >> patch has been in master for a few months, and it's two lines long, so
> >> it must be *somewhat* safe.
> >
> > Yes, a good idea.
>
> Any chance we can backport it now? I have hard-to-reproduce bug report
> from users of third-party code (company-mode plus elpy) which, I am
> told, is definitely fixed by applying this patch to emacs-26:
>
> https://github.com/jorgenschaefer/elpy/issues/1396#issuecomment-448142207
Risky... but okay, let's do it.
Thanks.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-12-19 16:07 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-14 14:03 bug#23980: 25.0.95; Events put in `unread-command-events' can be wrongly handled Chris Feng
2017-11-19 9:39 ` Chris Feng
2018-02-20 15:35 ` Dmitry Gutov
2018-02-20 18:37 ` Eli Zaretskii
2018-02-20 21:16 ` Dmitry Gutov
2018-02-21 4:22 ` Eli Zaretskii
2018-02-21 7:40 ` Chris Feng
2018-02-21 10:47 ` Dmitry Gutov
2018-02-21 12:23 ` Dmitry Gutov
2018-02-21 17:48 ` Eli Zaretskii
2018-02-21 21:45 ` Dmitry Gutov
2018-02-22 7:39 ` Eli Zaretskii
2018-12-19 13:54 ` Dmitry Gutov
2018-12-19 15:27 ` Eli Zaretskii
2018-12-19 16:07 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).