Text View widget : thread process can not change text buffer

[tissatussa]

your example widget codes are a great way to learn and understand how Owlkettle works, we can alter these codes to create own scripts. But now i encounter a bug (?) when using the Text View widget with a thread process : the content value of app.buffer.text can be read, but not written by it.

here's my script, which is a combination of https://github.com/can-lehmann/owlkettle/blob/main/examples/misc/threading.nim and https://github.com/can-lehmann/owlkettle/blob/main/examples/widgets/text_view.nim - my comments in this code give more details.

[ i mentioned this error also at the end of https://github.com/can-lehmann/owlkettle/issues/142 ]

import std/os
import owlkettle

viewable App:
  buffer: TextBuffer
  monospace: bool = false
  cursorVisible: bool = true
  editable: bool = true
  acceptsTab: bool = true
  indent: int = 0
  sensitive: bool = true
  sizeRequest: tuple[x, y: int] = (-1, -1) 
  tooltip: string = "" 

when compileOption("threads"):
  var thread: Thread[AppState]

proc threadProc(app: AppState) {.thread.} =
  while true:  # when this works, i want to output stdout of an external process this way..
    app.buffer.text = app.buffer.text & "HELLO" & "\n"  # gives error, but try-except does NOT catch it
    app.redrawFromThread()
    sleep(1000)

method view(app: AppState): Widget =
  result = gui:
    Window:
      title = "*** TEST ***"
      defaultSize = (1100, 600)

      HeaderBar {.addTitlebar.}:
        Button {.addLeft.}:
          style = [ButtonFlat]
          text = "GO"

          proc clicked() =
            when compileOption("threads"):
              createThread(thread, threadProc, app)

      ScrolledWindow:
        TextView:
          margin = 12
          buffer = app.buffer
          monospace = app.monospace
          cursorVisible = app.cursorVisible
          editable = app.editable
          acceptsTab = app.acceptsTab
          indent = app.indent
          sensitive = app.sensitive
          tooltip = app.tooltip
          sizeRequest = app.sizeRequest
          proc changed() = discard

when compileOption("threads") and (defined(gcOrc) or defined(gcArc)):
  let buffer = newTextBuffer()
  buffer.text = "first text line\n"
  brew(gui(App(buffer = buffer)))
else:
  quit "Compile with --threads:on and --gc:orc to enable threads"

i have no clue why this doesn't work .. it gives a severe error which can't be catched :

SIGSEGV: Illegal storage access. (Attempt to read from nil?) Segmentation fault (core dumped)

what's happening ?

[tissatussa]

btw. i think creating basic examples for the widgets, changing properties by events etc. are a great way to learn and have fun : combining them and adding other code, will please many (new) users .. by this, we can also see Nim in action : a lot can be done with just a few code lines.

[PhilippMDoerner]

I am not entirely well versed regarding whether we have event examples or not, but ever since the introduction of the playground, shouldn't most examples (or at the very least widget examples) have a way to play around with them like that? Mostly asking since I interpret your second post as recommendation to add some kind of property-changing mechanism to see the effects at runtime, which I think we already have (assuming I understand what you mean correctly).

See the scale.nim example for example (pun not intended). It has a menu button on the top right opening a popover-menu that lets you manipulate various AppState properties that get set on the widget itself, allowing you to see the effects first-hand.

I did not add that playground-feature to any of the widgets that I didn't touch since then (which is still the majority of examples I guess) so I guess it could be easily missed

[tissatussa]

can you see if my script is a proper combination of those two working example codes ? The value of a Label text can be changed, even using a thread, but app.buffer.text can't .. why ? Did you try my code to reproduce the severe error ?

[PhilippMDoerner]

I have not done anything with buffers yet, let alone TextBuffer. Neither have I written any amount of multi-thread owlkettle code (and barely write multi-threaded code generally), I'm lacking the knowledge here same as you.

I was solely remarking on the property-changing comment since that is something I wrote code for, namely the playground module.

[can-lehmann]

Your code looks correct to me. I am about 70% sure that this is a bug in the Nim programming language itself. Here is a much simpler example that also produces the same error:

import std/os
import owlkettle

viewable App:
  buffer: TextBuffer

method view(app: AppState): Widget =
  result = gui:
    Window:
      title = "Test"
      defaultSize = (1100, 600)

      HeaderBar {.addTitlebar.}:
        Button {.addLeft.}:
          style = [ButtonFlat]
          text = "Add"

          proc clicked() =
            app.buffer.text = app.buffer.text & "Hello"
            app.buffer.text = app.buffer.text & "world"

      ScrolledWindow:
        TextView:
          buffer = app.buffer
          proc changed() = discard

let buffer = newTextBuffer()
buffer.text = "first text line\n"
brew(gui(App(buffer = buffer)))

The reason why I believe that this is an issue in Nim itself is that it works as intended if I change the clicked callback to:

          proc clicked() =
            let buffer = app.buffer
            buffer.text = buffer.text & "Hello"
            buffer.text = buffer.text & "world"

This change should obviously make no difference.

[can-lehmann]

It also does not look like this is an issue with our =destroy/=copy hooks, as they are simply never called (TextBuffer is a ref object so that makes sense).

[tissatussa]

OK, i see .. i was hoping to find anyone around this project who could look into this code .. is it a bug ?

[can-lehmann]

Yes, probably in Nim itself. We would have to reduce the issue to a minimal example to be sure though.

[tissatussa]

i didn't yet try your code :

proc clicked() =
  let buffer = app.buffer
  buffer.text = buffer.text & "Hello"
  buffer.text = buffer.text & "world"

what a find ! what a workaround ! If this solves the issue, the Nim crew can profit to find their bug !?

[can-lehmann]

At least for me this workaround does not work with your multithreaded code. You might be able to get it to work with a bit of tweaking though.

[tissatussa]

mmm .. so that's the situation .. thanks anyhow, i will be trying some and maybe post updates here.

[can-lehmann]

I spent some time trying to reduce the code for my non-threaded example. I believe that the issue is caused by a memory management issue: When TextBuffer.text= is called, GTK fires the changed signal on the TextView. This causes a redraw which replaces the currently running Button.clicked event handler with a new one. I assume that Nim does not currently keep GC references to the closures of running procs, which is why the closure for the clicked event handler is destroyed while it is still running. I have opened an issue for this here: https://github.com/nim-lang/Nim/issues/23044

We can however work around this issue, by not calling the changed event handler, when the change is triggered programmatically. This is probably a good idea anyways as it makes the behavior more predictable.

[can-lehmann]

If you just remove the changed handler from your original example, it actually just works for me:

-           proc changed() = discard

Though as others have pointed out on the Nim forum, writing to a TextBuffer from another thread is not thread safe.

[tissatussa]

@can-lehmann thanks for researching this .. indeed several things are involved : GTK, Nim, Owlkettle .. i also saw the Nim forum reactions and writing to a TextBuffer from another thread is not thread safe .. i have to study this, also that client-server script concerning threads.

[can-lehmann]

Since the Nim issue is still open, I decided to implement a workaround: https://github.com/can-lehmann/owlkettle/commit/71b291792363583cb27fdf483d3a99f5f50346e5