brick icon indicating copy to clipboard operation
brick copied to clipboard

Brick applications occasionally start without any color attributes rendered

Open AndreiUlmeyda opened this issue 3 years ago • 26 comments

Hey there, great library!

I am currently building a simple cli application which will render a list of items, one on line each, where each item consists of an hBox containing three text widgets like so

hBox [ txt boldStuff, txt moreBoldStuff, txt boldAndColored ] (selected line)

There is always one selected line which is highlighted by being rendered boldface, all other lines have no style applied.

The two different styles are defined like so

selectionStyling = withStyle currentAttr bold :: Attr coloredStyling = withStyle (fg cyan) bold :: Attr

and then inserted into the attribute map

App { ..., appAttrMap = (const . attrMap currentAttr) [(attrName "selected", selectionStyling), (attrName "colored", coloredStyling)], ... }

Now, most of the times the app starts up the styling is applied as expected, a selected line is rendered boldface with the last section colored. All other lines stay plain.

But once every couple of dozen times the app will start with the colored styling missing. The selected line is still bold and the rest of the program seems to be working fine apart from that.

Perhaps I am handling the attribute map and the attribute 'currentAttr' incorrectly. And/Or maybe the order these the styles are applied depends on the evaluation order and does not compose with currentAttr too well.

Do you have any leads about what I may be missing?

Cheers! And thanks in advance,

Adrian

AndreiUlmeyda avatar Jul 15 '22 19:07 AndreiUlmeyda

Hi @AndreiUlmeyda - thanks for opening this. Do you happen to have a complete minimal test program that exhibits this behavior? If so, that would really help me investigate.

jtdaugherty avatar Jul 15 '22 19:07 jtdaugherty

Not a minimal one as of now, but I will try to supply one in the next few days. Thanks for the swift reply ;). The project it is occuring in isn't too big but it's also not hosted online atm.

AndreiUlmeyda avatar Jul 15 '22 19:07 AndreiUlmeyda

Okay, no problem. You might consider just starting with one of the demo programs since they're mostly pretty small.

jtdaugherty avatar Jul 15 '22 19:07 jtdaugherty

brick-github-issue-378.tar.gz

Howdy, here is the minimal example. I attached the entire repository of the stack project (a few MB because I probably committed a binary at some point like a dummy :>) Take a look at the branch 'styling-issue-minimal-example'.

Let me know if I can contribute anything else. Good luck and thanks for the effort.

AndreiUlmeyda avatar Jul 17 '22 10:07 AndreiUlmeyda

Thanks, @AndreiUlmeyda. About how much of the time would you say you experience the problem with this program? (I ran it about 30 times and it never seemed to misbehave.)

jtdaugherty avatar Jul 17 '22 16:07 jtdaugherty

Digging into this a bit more, I have a hypothesis about how this can happen. Vty detects the color mode of the terminal by querying terminfo. The implementation for the terminfo call that gets the color count is here. That call uses unsafePerformIO, which means that it could well be nondeterministic in its behavior. That suggests that it's conceivable that the capability query could sometimes yield an inconsistent answer depending on how the unsafe IO evaluation occurs. I'll look into this more to see if there's a workaround.

jtdaugherty avatar Jul 17 '22 17:07 jtdaugherty

I've just experimented a bit. The intervals for it occurring were ~85, ~40, ~50, 3, 23, 26, 13, 21, 23, ~50 number of startups.

The segfaults occurred at 2-3x that frequency.

Looks like this could be a real b**** to reproduce :/

AndreiUlmeyda avatar Jul 17 '22 17:07 AndreiUlmeyda

Segfaults on startup seem to occur when input is transmitted on the terminal before Vty is ready for it. That's a long-standing issue and I believe it's unrelated to the color problem.

jtdaugherty avatar Jul 17 '22 18:07 jtdaugherty

Related: https://github.com/judah/terminfo/issues/47

jtdaugherty avatar Jul 17 '22 18:07 jtdaugherty

@AndreiUlmeyda since I am having difficulty reproducing the behavior, would you be willing to try out this vty patch and see if the problem goes away in your situation? This would need to be applied to the vty package and then that modified package would need to be included in your build:

diff --git a/src/Graphics/Vty/Attributes/Color.hs b/src/Graphics/Vty/Attributes/Color.hs
index 41f82d3..3f92bee 100644
--- a/src/Graphics/Vty/Attributes/Color.hs
+++ b/src/Graphics/Vty/Attributes/Color.hs
@@ -125,7 +125,7 @@ detectColorMode termName' = do
     term <- catch (Just <$> Terminfo.setupTerm termName')
                   (\(_ :: Terminfo.SetupTermError) -> return Nothing)
     let getCap cap = term >>= \t -> Terminfo.getCapability t cap
-        termColors = fromMaybe 0 $ getCap (Terminfo.tiGetNum "colors")
+        termColors = fromMaybe 0 $! getCap (Terminfo.tiGetNum "colors")
     colorterm <- lookupEnv "COLORTERM"
     return $ if
         | termColors <  8               -> NoColor

jtdaugherty avatar Jul 17 '22 19:07 jtdaugherty

I will definitely do that. It will probably take a few days until I come around.

Until then, take care.

AndreiUlmeyda avatar Jul 17 '22 20:07 AndreiUlmeyda

Hey there again.

I tried the following

  1. check out https://github.com/jtdaugherty/vty into the root directory of the package
  2. Edit the line you suggested
  3. add - vty under 'packages' inside of stack.yaml
  4. stack clean
  5. stack build (some vty stuff gets compiled)

After that I tried again and the issue is still there, although it felt like I had to try for quite a while. Thats not enough information, though, to conclude thats something has changed.

Thanks for the suggestion, though. I will continue to help whenever I can. Meanwhile, the bug is not a dealbreaker for me, it rarely even registers as a nuisance. Still, it's probably preferrable to fix it at some point.

Cheers.

AndreiUlmeyda avatar Jul 21 '22 14:07 AndreiUlmeyda

@jtdaugherty maybe this Issue could be renamed to better match the current understanding of the problem?

We see this once in a while in Swarm, but I would not have guessed this to be the same Issue.

In Swarm, it's like running the application with defaultConfig {colorMode = Just NoColor}.

xsebek avatar Nov 11 '22 23:11 xsebek

I'm happy to edit the title. I didn't edit it in the past because I think it's a hypothesis that the problem reported here is the problem I think it is. :)

jtdaugherty avatar Nov 11 '22 23:11 jtdaugherty

Hi, I have created a small tweak to terminfo to try to prevent a potential race condition: https://github.com/judah/terminfo/compare/master...lock-curterm Could you please try it out and see if that resolves the issue? If so, we can look into whether that patch or another approach would be an appropriate fix.

judah avatar Nov 20 '22 01:11 judah

@judah thank you! I'll run matterhorn against this patch for a while and see if the problematic behavior surfaces. It's hard to reproduce even when it's there, though, so I don't know how to be sure that the patch helped.

jtdaugherty avatar Nov 20 '22 02:11 jtdaugherty

Incidentally I decided to try @AndreiUlmeyda's submitted demo program to see if I could reproduce this. After running that demo program a few hundred times, I did not see the bug surface a single time even without the latest change to attempt a fixed evaluation behavior! I also ran some of the simpler brick demo programs (which are only somewhat more sophisticated than the other one I ran here). After running some of them 400-500 times, again, I did not see a single problem. That's on GHC 9.2.x and 9.4.x, with brick somewhere between 0.79 and 1.4.

jtdaugherty avatar Nov 20 '22 03:11 jtdaugherty

(I also saw zero segfaults, contrary to the original report. Yay?)

jtdaugherty avatar Nov 20 '22 03:11 jtdaugherty

@judah given my results, I'm starting to think that at this point this may be worth closing and re-opening later if the problem comes up again. And when it does, I'll want to nail down as many details as possible in case there's something very particular about the combination of factors:

  • OS
  • Architecture
  • GHC version
  • Cabal version
  • cabal-install version
  • vty version
  • brick version
  • terminfo version
  • Maybe just go wild and capture dist-newstyle/cache/plan.json

Anything else come to mind that we should look at?

jtdaugherty avatar Nov 20 '22 03:11 jtdaugherty

Hi @jtdaugherty, could you wait a few days before closing, so that I could test it in Swarm? 🙂

Also, how did you test it @jtdaugherty and @AndreiUlmeyda?

I used a simple Bash for loop, but I wonder if doing it manually or maybe using screen would make a difference? 🤔 But I only run it like 30 times, which I take is not enough for the problem to show up.

xsebek avatar Nov 20 '22 11:11 xsebek

@xsebek I did my tests from within tmux (where I have seen the issue arise before) using something like for i in $(seq 20) ; do tmux new-window ... ; done from bash to fire up a large number of windows at a time. I'd then go through them all, checking for misbehavior, closing as I went. I ran many such batches to quickly get to hundreds of runs.

FYI I'm not in a hurry to close this; @xsebek before you test using any proposed fixes above, I'd love to know if you can reproduce the problem. If you can, I'd love to know the details above!

jtdaugherty avatar Nov 20 '22 19:11 jtdaugherty

@jtdaugherty If this helps, I've seen it happen to me approximately 5 times per 100 runs. No steps for reproduction.

refaelsh avatar Mar 07 '23 19:03 refaelsh

@jtdaugherty @judah

FWIW, I experienced regular segfaults when using Brick (at least 1 every 7-8 tries) that went away after using terminfo at the lock-curterm branch, so it seems like it may be worth releasing that change.

nicolodavis avatar May 18 '23 02:05 nicolodavis

@nicolodavis would you be willing to mention that over on terminfo, then? I had no idea that branch existed and I'd want @judah to weigh in on its fitness for release.

jtdaugherty avatar May 18 '23 03:05 jtdaugherty

(But that's great to hear, assuming that branch actually fixes this issue!)

jtdaugherty avatar May 18 '23 04:05 jtdaugherty

Sure thing, will post over there.

nicolodavis avatar May 18 '23 07:05 nicolodavis