Support #2660
evaluate if the NCURSES 5.7 "threading improvements" can be made to work for P2J such that auto_getch_refresh() is no longer needed
100%
Related issues
History
#1 Updated by Greg Shah almost 11 years ago
http://invisible-island.net/ncurses/ncurses.faq.html#multithread
Why does (fill in the blank) happen when I use two threads?
If you have a program which uses curses in more than one thread, you will almost certainly see odd behavior. That is because curses relies upon static variables for both input and output. Using one thread for input and other(s) for output cannot solve the problem, nor can extra screen updates help. This FAQ is not a tutorial on threaded programming.
Starting with ncurses 5.7, this implementation of curses provides the ability to configure and compile the library to help solve the problem by:
reducing the use of static variables (see curs_sp_funcs(3x)),
adding mutexes around the low-level terminal I/O,
changing global variables such as LINES to "getter" functions (see curs_opaque(3x), and
adding functions which an application can use to avoid those which rely upon global (or static) variables (see curs_threads(3x).
Almost all programs can be recompiled to use the "ncursest" or "ncursestw" libraries. Just recompiling is not enough to make a program thread-safe. As usual, some (re)design effort is probably needed.
The test-programs provided with ncurses (ncurses-examples) include a few which demonstrate this alternate configuration of the library: ditto, rain, worm.
#2 Updated by Greg Shah almost 11 years ago
-------- Forwarded Message --------
Subject: proposed extension API addition for ncurses 5.5
Date: Tue, 29 Aug 2006 12:30:37 -0400
From: Greg Shah <ges@goldencode.com>
To: dickey@invisible-island.net
Mr. Dickey,
I have an application which requires that a user can generate an
asynchronous notification (via CTRL-C) at any time, no matter what the
state of the input or output processing of the application. This
notification must be honored immediately by the application. Due to this
design requirement, we have implemented our application in 2 threads.
I fully understand that NCURSES is not thread safe, but I have found a
way to make it safe-enough in this particular case.
I use 1 thread for reading the keyboard via getch() and another thread
for all output processing. Any time the keyboard reading thread (which
is simply in an infinite loop of calling getch() with timeout set to 500
ms) happens to call into getch() at the same moment that the output
thread is processing a screen update there will be a problem. In
particular, getch() forces a refresh() before blocking on reading the
FIFO or terminal. Since access to the internal data structures of
ncurses is not protected in any way, calling a refresh() at
(essentially) random times will sometimes force the output buffer to the
terminal at a moment when it is in an inconsistent state (because it is
in the middle of being updated by another thread). This causes
unfinished escape sequences etc... to be pushed out, leaving the
terminal looking corrupted.
My solution is to add a simple extension to NCURSES 5.5:
void auto_getch_refresh(bool)
This simply sets a static flag inside lib_getch.c. This flag is checked
inside the wgetch_should_refresh macro to allow the disabling of
refresh() during getch(). This getch_refresh flag defaults to TRUE
which is completely compatible with the current NCURSES implementation.
So in all current usage, there is no difference in behavior. In the
case where one wishes to disable refresh during getch(), this is now
possible. In particular, we found that our 2 threaded case where 1
thread is dedicated to getch() is now safe.
Some thoughts:
1. This is very low risk.
2. In the case where this new function is never called, this doesn't
change the behavior in regards to standards or interface compliance.
3. The performance cost is very low (on an Intel CPU, only 3 additional
instructions per getch()).
4. The memory footprint of the data is only an extra 4 bytes per process.
I am hoping this will be acceptable for inclusion in the next release of
NCURSES.
Thank you for your time. Please let me know if you have any questions
or comments.
Greg Shah
#3 Updated by Greg Shah almost 11 years ago
-------- Forwarded Message --------
Subject: Re: proposed extension API addition for ncurses 5.5
Date: Tue, 29 Aug 2006 20:10:06 -0400 (EDT)
From: Thomas Dickey <dickey@his.com>
To: Greg Shah <ges@goldencode.com>
CC: dickey@invisible-island.net
On Tue, 29 Aug 2006, Greg Shah wrote:
My solution is to add a simple extension to NCURSES 5.5:
I'd suppose it would be more straightforward to add mutexes and measure
the performance than to add a special case. What you're describing is
a non-threadsafe version of one of the likely places for adding a mutex.
void auto_getch_refresh(bool)
This simply sets a static flag inside lib_getch.c. This flag is checked
inside the wgetch_should_refresh macro to allow the disabling of refresh()
during getch(). This getch_refresh flag defaults to TRUE which is completely
compatible with the current NCURSES implementation. So in all current usage,
there is no difference in behavior. In the case where one wishes to disable
refresh during getch(), this is now possible. In particular, we found that
our 2 threaded case where 1 thread is dedicated to getch() is now safe.
But if two threads were to call wgetch(), that wouldn't work.
--
Thomas E. Dickey
http://invisible-island.net
ftp://invisible-island.net
#4 Updated by Greg Shah almost 11 years ago
-------- Forwarded Message --------
Subject: Re: proposed extension API addition for ncurses 5.5
Date: Wed, 30 Aug 2006 07:22:19 -0400
From: Greg Shah <ges@goldencode.com>
To: Thomas Dickey <dickey@his.com>
CC: dickey@invisible-island.net
Yes, this is all true. And having multiple threads in output operations
is similarly problematic. But a real thread-safe implementation is
quite a large undertaking. Every shared resource and data structure
would have to be protected. To do it right, a great deal of analysis is
needed as to the granularity of the mutexes, possible deadlock/race
conditions... and access to most data structures would best be hidden
behind new internal interfaces (getters/setters or the like) to allow
the mutex usage to be consistent without hard coding the mutex access
everywhere. For example, even reading a flag from the WINDOW structure
would require protection.
I don't have the time (does anyone?) to do a full thread-safe
implementation. So my approach is a compromise to be safe-enough while
minimizing the effort and risk.
Thanks,
Greg
#5 Updated by Greg Shah almost 11 years ago
I never heard back from that reply. BUT then they added some dual threading support in 5.7. NCURSES 5.7 came out on November 2, 2008, 2 years after my email conversation with Thomas Dickey. It is possible that the new approach handles our dual threading needs as well as our approach. The mutexes are certainly slower than my approach, but if the difference can't be seen by the user AND it is just as safe/reliable, then we should move to the new approach and eliminate the need for patching NCURSES.
This task is intended to test out the viability of this approach and implement the change if it is OK.
#6 Updated by Greg Shah almost 11 years ago
- Target version set to Deployment and Management Improvements
#7 Updated by Greg Shah almost 11 years ago
NCURSES 5.9 release notes includes:
This extends support for threaded applications by providing a new API which eliminates the need for a global screen-pointer.
#8 Updated by Greg Shah over 9 years ago
- Target version deleted (
Deployment and Management Improvements)
#9 Updated by Greg Shah over 6 years ago
- Related to Support #4549: reduce/eliminate installation dependencies added
#10 Updated by Greg Shah over 5 years ago
- Related to Support #5167: using static linking to eliminate the need to patch the system-wide ncurses added
#11 Updated by Greg Shah over 1 year ago
- Related to Bug #7657: 8-bit character entry problem in ChUI added
#12 Updated by Eugenie Lyzenko over 1 year ago
Update for experiments with pthreads enabled libp2j.so. Some preconditions and info.
1. The NCURSES base is as of revision 6.3+. This includes some threading improvements implemented in original package.
2. The static NCURSES library should be remake with configure options:
./configure --with-termlib CFLAGS='-fPIC -O2' --with-abi-version=6 --with-pthread --enable-pthreads-eintr --enable-widec
This initiates required internal variables and changed logic inside
NCURSES and mandatory.3. The
makefile for native FWD module should be changed to enable ptreads (assuming NCURSES is wide char capable):
...
# linux section
ifeq "$(OS)" "Linux"
override CFLAGS+=-fpic
# NCURSES library is a requirement in the project anyway so the C code
# calls functions in that interface directly instead of exec'ing command
# line utilities for the same purpose (to avoid the hard requirement of
# having extra utility programs installed in addition to P2J); this is
# the reason why libp2j depends on libncurses:
override LDFLAGS+=-ldl -lutil
ifdef NCURSES_FWD_STATIC
override INCLUDES+=-I${NCURSES_FWD_STATIC}/include
ifdef NCURSES_FWD_WIDE_CHARS
- override LDFLAGS+=-L${NCURSES_FWD_STATIC}/lib -l:libncursesw.a -l:libtinfow.a -z defs
+ override LDFLAGS+=-L${NCURSES_FWD_STATIC}/lib -l:libncursestw.a -l:libtinfotw.a -z defs -lpthread
override CFLAGS+=-DNCURSES_FWD_WIDE_CHARS
else
override LDFLAGS+=-L${NCURSES_FWD_STATIC}/lib -l:libncurses.a -l:libtinfo.a -z defs
endif
else
ifdef NCURSES_FWD_WIDE_CHARS
- override LDFLAGS+=-lncursesw
+ override LDFLAGS+=-lncursestw -lpthread
override CFLAGS+=-DNCURSES_FWD_WIDE_CHARS
else
override LDFLAGS+=-lncurses
endif
endif
# this option is valid in Linux but not in Solaris
override RMCMD+=v
endif
...
4. The final change s to turn
auto_getch_refres() off (terminal_linux.c):... nonl(); - auto_getch_refresh(FALSE); // disable sync during getch() def_prog_mode(); // save our state (after our mode changes) ...
This combination gives interesting results. We have clean ChUI regression testing cycle. So the question is what is the knows scenario that will fail for not patched NCURSES? If there is no one then we can probably do not need to patch NCURSES anymore.
#13 Updated by Eugenie Lyzenko over 1 year ago
Made a set if ChUI regression testing. A lot enough I guess to make conclusion. The not patched NCURSES (disabled usage of the getch_refresh flag) has clean results, no regressions.
#14 Updated by Eugenie Lyzenko over 1 year ago
Eugenie Lyzenko wrote:
Made a set if
ChUIregression testing. A lot enough I guess to make conclusion. The not patchedNCURSES(disabled usage of thegetch_refreshflag) has clean results, no regressions.
The addition. The pthreads is on for these testing (NCURSES has slightly different internal logic).
#16 Updated by Greg Shah over 1 year ago
#307 is the original issue that made the requirement for auto_getch_refresh().
The
pthreadsis on for these testing (NCURSEShas slightly different internal logic).
Does this different logic implicitly make NCURSES thread-safe (i.e. protects the internal data structures when accessed from different threads at the same time)?
#17 Updated by Eugenie Lyzenko over 1 year ago
Greg Shah wrote:
#307 is the original issue that made the requirement for
auto_getch_refresh().The
pthreadsis on for these testing (NCURSEShas slightly different internal logic).Does this different logic implicitly make
NCURSESthread-safe (i.e. protects the internal data structures when accessed from different threads at the same time)?
I would like to say yes. But I can not(for 100%). What I see is the NCURSES inits and uses internal mutexes to serialize some data access. But I think this is still not exactly what we want from this library. The both input and output uses mutex locks in pthread capable mode. But seems like the objects are different. Another word the key reader uses one mutex while screen update uses another one. So this can be considered as thread safe for two threads that attempt to read or write simultaneously from/to the same terminal. This is not the case when one thread is reading key and another one does the screen update. But I need to dig more inside NCURSES to confirm this theory.
So to see the sync issue I need to have working m***c ChUI application? We may need to have standalone testcase that reproduces this issue.
#18 Updated by Greg Shah over 1 year ago
I would like to say yes. But I can not(for 100%). What I see is the
NCURSESinits and uses internal mutexes to serialize some data access. But I think this is still not exactly what we want from this library. The both input and output uses mutex locks in pthread capable mode. But seems like the objects are different. Another word the key reader uses one mutex while screen update uses another one. So this can be considered as thread safe for two threads that attempt to read or write simultaneously from/to the same terminal. This is not the case when one thread is reading key and another one does the screen update. But I need to dig more insideNCURSESto confirm this theory.
Yes, this would be the next step for this task.
So to see the sync issue I need to have working m***c
ChUIapplication? We may need to have standalone testcase that reproduces this issue.
Yes, the problem was originally found in the application used for ChUI regression testing. However, the basic idea can probably be duplicated in Hotel ChUI. Key processing at the same time as screen updates caused corruption because getch() forced a screen refresh. The code assumed that everything was single threaded so it was safe to do.
#19 Updated by Eugenie Lyzenko over 1 year ago
Greg Shah wrote:
I would like to say yes. But I can not(for 100%). What I see is the
NCURSESinits and uses internal mutexes to serialize some data access. But I think this is still not exactly what we want from this library. The both input and output uses mutex locks in pthread capable mode. But seems like the objects are different. Another word the key reader uses one mutex while screen update uses another one. So this can be considered as thread safe for two threads that attempt to read or write simultaneously from/to the same terminal. This is not the case when one thread is reading key and another one does the screen update. But I need to dig more insideNCURSESto confirm this theory.Yes, this would be the next step for this task.
OK. Made extra code reading for NCURSES source package. The result is positive. Both key reading and screen update from adding char finally uses the same call to wrefresh() which in turn has mutex lock for update object. So far the NCURSES has two locks protection for simultaneous read from screen and write to screen for multiple threads that reading key and writing data to screen. The protection is even stronger than I have expected previously. But to use it the --with-pthread --enable-pthreads-eintr must be enabled when building static NCURSES. The libp2j.so should also be compiled with -lpthread.
This is the Linux case build. Still not investigated how it works for Solaris.
So to see the sync issue I need to have working m***c
ChUIapplication? We may need to have standalone testcase that reproduces this issue.Yes, the problem was originally found in the application used for ChUI regression testing. However, the basic idea can probably be duplicated in Hotel ChUI. Key processing at the same time as screen updates caused corruption because
getch()forced a screen refresh. The code assumed that everything was single threaded so it was safe to do.
I have tested the Hotel ChUI application with original NCURSES built with pthread option enabled(no patch in use). The result is the screens are correct no matter how fast I press the keys causing screen update. I tried to get screen corruption but no success.
So for now my conclusion is we can get rid of the NCURSES patching in Linux FWD build. But TERMINFO is need to be updated anyway because it is independent task.
#20 Updated by Greg Shah over 1 year ago
I have tested the Hotel ChUI application with original NCURSES built with pthread option enabled(no patch in use). The result is the screens are correct no matter how fast I press the keys causing screen update. I tried to get screen corruption but no success.
To confirm that this testing is sufficient, please retest with pthread disabled and no auto_getch_refresh(). If you can recreate the corruption using your same testing, then we can say that the conclusion is correct.
#21 Updated by Eugenie Lyzenko over 1 year ago
Greg Shah wrote:
I have tested the Hotel ChUI application with original NCURSES built with pthread option enabled(no patch in use). The result is the screens are correct no matter how fast I press the keys causing screen update. I tried to get screen corruption but no success.
To confirm that this testing is sufficient, please retest with pthread disabled and no
auto_getch_refresh(). If you can recreate the corruption using your same testing, then we can say that the conclusion is correct.
The result is strange. I can not recreate the failure for no auto_getch_refresh() with Hotel ChUI demo application. Trying to get this with ChUI regression harness.
May be the failure is specific to particular NCURSES version? Say before 5.7. Agreed until we get a failure we can not say there is a way to drop NCURSES patching. So continue working.
#22 Updated by Eugenie Lyzenko over 1 year ago
Status update.
I have finished 2 rounds of main loop for ChUI regression testing with libp2j.so that does not use NCURSES patch. And we have the failures with screen content. The failed tests are different in different rounds. But we can see the problem now the NCURSES patch was designed to fix. The FWD tested was not wide char capable, regular trunk version.
The CTRL-C tests are running without issues. And Hotel ChUI is also not representative.
The NCURSES version under testing is 6.3
So the next step will be to rebuild this FWD with pthread capable version and repeat the tests. If there will be no problem - we have a cure to drop NCURSES patch.
#23 Updated by Eugenie Lyzenko over 1 year ago
Status update. I have executed one full ChUI regression cycle for FWD compiled with pthread capable NCURSES. The result is - no regression for same native code used to recreate screen garbage issue with legacy compiled FWD. So far the NCURSES usage us the same (no auto_getch_refresh active patch). But using pthread options in building static NCURSES gives clean processing for concurrent get char and output. The wide char capability was not used with these tests, only trunk build is involved.
I have two sequential problematic loops (without pthreads) and one trouble-free loop for pthreads active NCURSES. Now the second run for pthread is in progress. In the case it will be OK I guess we have consistent base to tell the NCURSES patch can be dropped if we will use pthread while building static NCURSES making this library enough thread safe, At least for our purpose.
What do you think? I guess it is a good news.
#24 Updated by Greg Shah over 1 year ago
Yes, that does sound quite good.
How do we check to see if pthreads is already on in an existing NCURSES build? I'm wondering if we still have to build NCURSES or if the version in common platforms (like Ubuntu) already has it activated.
#25 Updated by Eugenie Lyzenko over 1 year ago
Greg Shah wrote:
Yes, that does sound quite good.
How do we check to see if
pthreadsis already on in an existingNCURSESbuild? I'm wondering if we still have to buildNCURSESor if the version in common platforms (like Ubuntu) already has it activated.
Do you mean the NCURSES that is system wide for give Ubuntu, not static we use to build FWD? I think this is uncommon to have one with pthread enabled. For now I can tell such library should have t suffix in library name. Similar to having w suffix for wide char option. So for example if the regular one is libncurses.so.X.Y the pthread capable version will be libncursest.so.X.Y.
And I think by default the pthread version is not installed for system wide usage. So we either have to use our own static version or install t version manually(with possible conflicts between regular and t version).
It t version is better in any case and OS is multi-threaded there should be a reason why it is not used by default.
#26 Updated by Greg Shah over 1 year ago
Eugenie Lyzenko wrote:
Greg Shah wrote:
Yes, that does sound quite good.
How do we check to see if
pthreadsis already on in an existingNCURSESbuild? I'm wondering if we still have to buildNCURSESor if the version in common platforms (like Ubuntu) already has it activated.Do you mean the
NCURSESthat is system wide for give Ubuntu, not static we use to buildFWD?
Yes, exactly. The idea is that if it already exists we can use it.
And I think by default the
pthreadversion is not installed for system wide usage. So we either have to use our own static version or installtversion manually(with possible conflicts between regular andtversion).
Yes, I have the regular one and the wide one installed on my dev system but no "t" version. OK, then we just plan to continue using the local build + static linking approach but without any patching.
#27 Updated by Eugenie Lyzenko over 1 year ago
Greg,
What is the priority for the task? Should I create 2660a branch to commit the required changes and move to use "t" version of static NCURSES right now? Or we could combine this fix with solution for having private TERMINFO option included in FWD?
#28 Updated by Greg Shah over 1 year ago
It can be combined.
#29 Updated by Eugenie Lyzenko over 1 year ago
- Related to Support #5568: implement application-specific TERMINFO database overrides added
#30 Updated by Eugenie Lyzenko over 1 year ago
Greg Shah wrote:
It can be combined.
OK.
#31 Updated by Greg Shah over 1 year ago
- % Done changed from 0 to 100
- Status changed from New to Test
- Assignee set to Eugenie Lyzenko
The changes for this task were implemented in task branch 5568a, which was merged to trunk in revision 15728.
Please see:
Client Installation
Building FWD From Source
The common text is in NCURSES which references these:
Patching NCURSES
Patching NCURSES Using Static Linking
Using Thread-Safe NCURSES with Static Linking
#32 Updated by Roger Borrello about 1 month ago
- Related to Bug #11515: Patching NCURSES Using Static Linking on Ubuntu 24.04.4 LTS (Noble Numbat) added