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

[RFC] make unibi_out output never cross buffer boundaries #7676

Merged
merged 2 commits into from Dec 6, 2017

Conversation

Projects
None yet
3 participants
@florolf
Contributor

florolf commented Dec 2, 2017

TL;DR: cdfaecb caused a regression w.r.t. the problem fixed in #5436. This will hopefully fix this once and for all. See the commit messages for detailed reasoning.

@justinmk

This comment has been minimized.

Member

justinmk commented Dec 2, 2017

Greatly appreciated (again) :)

see also #7381 (comment)

The QuickBuild CI failures are unrelated.

size_t available = sizeof(data->buf) - data->bufpos;
if (out_ctx->overflow)
return;

This comment has been minimized.

@justinmk

justinmk Dec 2, 2017

Member

linter won't like this, i'll change it during the merge if CI passes.

data->bufpos += len;
}
static void limited_out(void *ctx, const char *str, size_t len)

This comment has been minimized.

@justinmk

justinmk Dec 2, 2017

Member

Any reason not to replace out() with this entirely? And maybe store the state on TUIData instead of the new limited_out_ctx struct.

This comment has been minimized.

@florolf

florolf Dec 2, 2017

Contributor

I didn't want to modify the current users of out(), both because this slightly changes the semantics (possibly causing flushes at different points) and requires them to pass a context struct.

The semantics part shouldn't be an issue though, so one could reimplement out() using limited_out() which would reduce duplicated code. I'll do that.

Rolling llimited_out_ctx into TUIData feels slightly wrong because this is local state, but I have no strong opinion on this. If you think this is better stored as part of TUIData, I'll move it there. Your call.

This comment has been minimized.

@justinmk

justinmk Dec 2, 2017

Member

The semantics part shouldn't be an issue though, so one could reimplement out() using limited_out() which would reduce duplicated code. I'll do that.

Ok, will wait for that before merging.

Rolling llimited_out_ctx into TUIData feels slightly wrong because this is local state, but I have no strong opinion on this. If you think this is better stored as part of TUIData, I'll move it there. Your call.

Let's leave it as you have it. TUIData has fields like bufpos, but what you have is probably clearer and otherwise harmless.

@marvim marvim added the RFC label Dec 2, 2017

@florolf florolf force-pushed the florolf:atomic-unibilium branch 2 times, most recently from 811e93d to 93f1303 Dec 5, 2017

@florolf

This comment has been minimized.

Contributor

florolf commented Dec 5, 2017

Only got to looking into this again today, sorry.

This is a new approach: The patches now introduce a "cork" state which prevents the buffer from being flushed (in cases where it ordinarily would be flushed, an overflow flag is raised and no further output is done). This reduces the code duplication somewhat.

When looking at the other users of out I realized that tui_set_title is another candidate for corruption by visibility toggling, but fixing this with the current infrastructure is a little more difficult. Once could leave the "do not toggle"-option of flush_buf in and use that in tui_set_title, but that would make things more complex. We could also simply truncate the title, as a 64k char long title probably doesn't make much sense. Either way, this probably is an issue for another PR.

@florolf florolf force-pushed the florolf:atomic-unibilium branch from 93f1303 to 0c8de33 Dec 5, 2017

florolf added some commits Nov 29, 2017

tui: never flush buffers in midst of unibilium output
e838452 fixed an issue where long (true color) escape sequences got
interrupted by the cursor visibility toggling caused by buffer flushes.

cdfaecb introduces a new issue which causes similar problems: While
the old buffer flushing code appended the cursor visibility escapes to
the buffer before/after flushing, the new code effectively prepends the
sequences.

Assume the following sequence of events occurs:

 - A long escape code is issued using unibi_out when the buffer is
   almost full
 - out() gets called for a prefix of that escape code, causing the
   buffer to fill up
 - flush_buf(ui, false) is called and (correctly) does not insert any
   cursor toggling escapes
 - The rest of the escape code is written into the now empty buffer
 - At some later point, some other part of nvim calls flush_buf(ui,
   true), which then toggles the cursor, corrupting the escape code

This could possibly also be fixed by tracking the state of the buffer
(i.e. does it contain a partially output escape code?), but this seems
fragile in the same way e838452 turned out to be.

The root cause for all these problems is the mismatch between nvim's
(implicit) assumption that the buffer is flushable at any point in time
and the non-atomicity of unibilium's character based callback interface.
The proper fix (without modifying unibilium) is to ensure nvim's
assumption about the buffer state holds at all times.

To that end, add a "cork" flag which ensures one unibi_out-call never
splits its output across a buffer flush; if an escape code does not fit
into the current buffer, flush it without any part of the escape code in
it and insert the whole escape code in the emptied buffer. This is a
little more complex because it modifies the buffer in place rather than
printing into another buffer, checking the remaining space in the
terminal buffer and then memcpy'ing it.
tui: always hide cursor when flushing
The previous commit ensures that we can never flush the buffer in a
state where toggling cursor visibility can corrupt other escape codes.
Thus, we can remove the workaround added as part of e838452, simplyfing
the code and hiding the cursor on more occasions.

@florolf florolf force-pushed the florolf:atomic-unibilium branch from 0c8de33 to 54087d8 Dec 5, 2017

@florolf

This comment has been minimized.

Contributor

florolf commented Dec 5, 2017

Sorry for the push noise, I had a long day. Should be okay now.

@justinmk justinmk merged commit a58c909 into neovim:master Dec 6, 2017

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
QuickBuild Build pr-7676 finished with status SUCCESSFUL
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.01%) to 77.472%
Details

@justinmk justinmk removed the RFC label Dec 6, 2017

justinmk added a commit that referenced this pull request Jun 11, 2018

NVIM v0.3.0
FEATURES:
3cc7ebf #7234 built-in VimL expression parser
6a7c904 #4419 implement <Cmd> key to invoke command in any mode
b836328 #7679 'startup: treat stdin as text instead of commands'
58b210e :digraphs : highlight with hl-SpecialKey #2690
7a13611 #8276 'startup: Let `-s -` read from stdin'
1e71978 events: VimSuspend, VimResume #8280
1e7d5e8 #6272 'stdpath()'
f96d99a #8247 server: introduce --listen
e8c39f7 #8226 insert-mode: interpret unmapped META as ESC
98e7112 msg: do not scroll entire screen (#8088)
f72630b #8055 let negative 'writedelay' show all redraws
5d2dd2e win: has("wsl") on Windows Subsystem for Linux #7330
a4f6cec cmdline: CmdlineEnter and CmdlineLeave autocommands (#7422)
207b7ca #6844 channels: support buffered output and bytes sockets/stdio

API:
f85cbea #7917 API: buffer updates
418abfc #6743 API: list information about all channels/jobs.
36b2e3f #8375 API: nvim_get_commands
273d2cd #8329 API: Make nvim_set_option() update `:verbose set …`
8d40b36 #8371 API: more reliable/descriptive VimL errors
ebb1acb #8353 API: nvim_call_dict_function
9f994bb #8004 API: nvim_list_uis
3405704 #7520 API/UI: forward option updates to UIs
911b1e4 #7821 API: improve nvim_command_output

WINDOWS OS:
9cefd83 #8084, #8516 build/win: support MSVC
ee4e1fd win: Fix reading content from stdin (#8267)

TUI:
ffb8904 #8309 TUI: add support for mouse release events in urxvt
8d5a46e #8081 TUI: implement "standout" attribute
6071637 TUI: support TERM=konsole-256color
67848c0 #7653 TUI: report TUI info with -V3 ('verbose' >= 3)
3d0ee17 TUI/rxvt: enable focus-reporting
d109f56 #7640 TUI: 'term' option: reflect effective terminal behavior

FIXES:
ed6a113 #8273 'job-control: avoid kill-timer race'
4e02f1a #8107 'jobs: separate process-group'
451c48a terminal: flush vterm output buffer on pty output #8486
5d6732f :checkhealth fixes #8335
53f11dc #8218 'Fix errors reported by PVS'
d05712f inccommand: pause :terminal redraws (#8307)
51af911 inccommand: do not execute trailing commands #8256
84359a4 terminal: resize to the max dimensions (#8249)
d49c1dd #8228 Make vim_fgets() return the same values as in Vim
60e96a4 screen: winhl=Normal:Background should not override syntax (#8093)
0c59ac1 #5908 'shada: Also save numbered marks'
ba87a2c cscope: ignore EINTR while reading the prompt (#8079)
b1412dc #7971 ':terminal Enter/Leave should not increment jumplist'
3a5721e TUI: libtermkey: force CSI driver for mouse input #7948
6ff13d7 #7720 TUI: faster startup
1c6e956 #7862 TUI: fix resize-related segfaults
a58c909 #7676 TUI: always hide cursor when flushing, never flush buffers during unibilium output
303e1df #7624 TUI: disable BCE almost always
249bdb0 #7761 mark: Make sure that jumplist item will not have zero lnum
6f41ce0 #7704 macOS: Set $LANG based on the system locale
a043899 #7633 'Retry fgets on EINTR'

CHANGES:
ad60927 #8304 default to 'nofsync'
f3f1970 #8035 defaults: 'fillchars'
a6052c7 #7984 defaults: sidescroll=1
b69fa86 #7888 defaults: enable cscopeverbose
7c4bb23 defaults: do :filetype stuff unless explicitly "off"
2aa308c #5658 'Apply :lmap in macros'
8ce6393 terminal: Leave 'relativenumber' alone (#8360)
e46534b #4486 refactor: Remove maxmem, maxmemtot options
131aad9 win: defaults: 'shellcmdflag', 'shellxquote' #7343
c57d315 #8031 jobwait(): return -2 on interrupt also with timeout
6452831 clipboard: macOS: fallback to tmux if pbcopy is broken #7940
300d365 #7919 Make 'langnoremap' apply directly after a map
ada1956 #7880 'lua/executor: Remove lightuserdata'

INTERNAL:
de0a954 #7806 internal statistics for list impl
dee78a4 #7708 rewrite internal list impl
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment