Add stackTrace property to fiber

[mode777]

I was writing a small test runner and noticed that after calling fiber.try, you can only obtain the error text but not the location where the error occured, which makes debugging very cumbersome.

I added fiber.stackTrace in addition to fiber.error. The stack trace is formated on the fly, which means there is no additional performance overhead if you don't use the property.

This also works for fibers which are yielded. It will return null if the fiber hasn't run yet or returned already.

I added two additional tests to show how it works.

var fiber = Fiber.new {
  "s".unknown
}

System.print(fiber.stackTrace) // expect: null
System.print(fiber.try()) // expect: String does not implement 'unknown'.
System.print(fiber.stackTrace) // expect: at new(_) block argument:(main) line 2\n

EDIT: Looks like some commits show up here that should be in main already. Basically, I only want
e3f1774 and ad05671. Anyone knows some git magic on how to fix that?

[ChayimFriedman2]

If this is a bug, don't try(): let the fiber kill your program and the stack trace will be printed.

[ruby0x1]

often you want resilience as well as information, which this PR aims for.

[ChayimFriedman2]

Logging and so?

[ruby0x1]

This is not resilient:

let the fiber kill your program

[mode777]

I think there are valid use cases, where you want a detailed error report without killing the vm.

• A test runner that should continue when a test fails
• A scheduler which runs several fibers in parallel
• A web server that will report an error if one request fails

[ChayimFriedman2]

A web server that will report an error if one request fails

Logging and so?

Anyway, remove what I said 😄 You're right.

[mhermier]

Incidentally, doest it work on new fiber and yield fiber? And does it out something useful?

[mhermier]

It would be good to try to compress the stack trace in case of infinite direct recursion. Since the trace as limited output, it possible to not see where the error started. In addition if the stack trace get trunckated there should be an indication of the situation, so the user does not get confused.

[mode777]

@mhermier It returns null for a new Fiber and works correctly on a yielded Fiber. The current implementation will stop walking the stack after the 16kb buffer was filled.

[mhermier]

(sorry for the noise) Maybe a better solution would be to allow to iterate on stack frames entries, and let wren make the string. It is suboptimal, may have memory implications, but it should be easier to let the user customize the stack trace output.

[ChayimFriedman2]

"int" the output?? "in" the output... :P

[mode777]

Thanks ;)

[ChayimFriedman2]

We probably better with allocating the string on the heap from the start. This will save us a copy and also avoid allocating much memory on the stack, which can cause a stack overflow.

[mode777]

I don't see the benefit. wrenNewString will copy the memory in any case. As for the risk of a stack overflow: I'm not experienced enough in C to give an estimate of it's likelyness.

[ChayimFriedman2]

The likelihood depends mostly on compile flags. I found https://softwareengineering.stackexchange.com/a/310659, which says that 16kb are probably fine.

As for the copy, I meant to allocate a ObjString directly, then write to the space. I know this is a space for problems because we touch low level details.

[mode777]

I like the idea of the ObjString and will have a look into it. I probably have to think of hashing the string myself as well

[ChayimFriedman2]

Maybe a better solution would be to allow to iterate on stack frames

They meant for debugging and not for text processing, IMO. You can process the stack trace and find the function names etc., that reminds me what angularjs used to do... :P

[mode777]

(sorry for the noise) Maybe a better solution would be to allow to iterate on stack frames entries, and let wren make the string. It is suboptimal, may have memory implications, but it should be easier to let the user customize the stack trace output.

I thought about that but felt that the additional complexity was not worth it

[mhermier]

Yes but that info can be used in plain text, viewed inside a debugger and displayed in columns in a table, the user can make a stack trace formatter more to it's liking, or whatever. And more importantly, we have access to the whole trace, and not possibly a truncated version of it.

[mode777]

Yes but that info can be used in plain text, viewed inside a debugger and displayed in columns in a table, the user can make a stack trace formatter more to it's liking, or whatever. And more importantly, we have access to the whole trace, and not possibly a truncated version of it.

I see the benefits too, I just don't see a simple way of doing it. This means we would need to make some kind of "StackIterator" available that let's you iterate "StackFrame" objects both exposed to wren. Right?

[mhermier]

A simple solution is to do like string does with StringByteSequence. Do something like:

class Fiber {
  foreign iterateStackFrames_(iterator)
  foreign moduleToStringAt_(iterator)
  foreign functionToStringAt_(iterator)
  foreign lineAt_(iterator)

  stackTrace { StackStrace.new(this) }
}

// Lazy implementation example
class StackFrame {
  construct new_(fiber, iterator) {
    _fiber = fiber
    _iterator = iterator
  }

  module  { _fiber.moduleToStringAt_(_iterator) }
  function { _fiber.functionToStringAt_(_iterator) }
  line        { _fiber.lineAt_(_iterator) }

  toString { "at %(module):%(function ) line %( line )" }
}

class StackTrace is Sequence {
  construct new(fiber) {
    _fiber = fiber
  }

  iterate(iterator) { _fiber.iterateStackFrames_(iterator) }
  iteratorValue(iterator) { StackFrame.new_(_fiber, iterator) }

  toString { joint("\n") }
}

The implementation can be lazy or not. Going not lazy has the advantage of being able to compare stacks/make them easy to compare trivially, and could be helpfull in case of implementing a debugger that allows to trace instructions. But it is not an absolute requirement, since if we have access to the data we can make a copy of it.

[mhermier]

If we don't want to bloat core, another possibility is to move this to an extra module and move the foreign functions to StackFrame and change arguments to (fiber, iterator) in the signature.

[mode777]

A simple solution is to do like string does with StringByteSequence. Do something like:

class Fiber {
  foreign iterateStackFrames_(iterator)
  foreign moduleToStringAt_(iterator)
  foreign functionToStringAt_(iterator)
  foreign lineAt_(iterator)

  stackTrace { StackStrace.new(this) }
}

// Lazy implementation example
class StackFrame {
  construct new_(fiber, iterator) {
    _fiber = fiber
    _iterator = iterator
  }

  module  { _fiber.moduleToStringAt_(_iterator) }
  function { _fiber.functionToStringAt_(_iterator) }
  line        { _fiber.lineAt_(_iterator) }

  toString { "at %(module):%(function ) line %( line )" }
}

class StackTrace is Sequence {
  construct new(fiber) {
    _fiber = fiber
  }

  iterate(iterator) { _fiber.iterateStackFrames_(iterator) }
  iteratorValue(iterator) { StackFrame.new_(_fiber, iterator) }

  toString { joint("\n") }
}

The implementation can be lazy or not. Going not lazy has the advantage of being able to compare stacks/make them easy to compare trivially, and could be helpfull in case of implementing a debugger that allows to trace instructions. But it is not an absolute requirement, since if we have access to the data we can make a copy of it.

Thats also what I was thinking of. I think C# does a similar thing. I have a few concerns about this. Bloating core and the overall code base is one of them, I also think we will need to exchange the iterator with a pre-filled list. Otherwise we would be able to run the fiber between iterations, which sounds like a recipe for disaster.

I would definitely be willing to give such an implementation a try but I would like to hear some more opinions on that before putting any more work in it.

[mhermier]

One solution to mitigate the bload is to lazy import StackTrace by doing:

class Fiber {
  stacktrace {
    import "what_ever_module_name" for StackTrace
    return StackTrace.new(this)
  }
}

[mode777]

One solution to mitigate the bload is to lazy import StackTrace by doing:

class Fiber {
  stacktrace {
    import "what_ever_module_name" for StackTrace
    return StackTrace.new(this)
  }
}

Possible but using a module external of core for accessing Wren internals seems a little odd. It might be possible to expose stack-walking functionality to the public C Api and let people implement the functionality themselves - but this seems overly complex to me

[mhermier]

Just thought about a simpler implementation that don't need a separated module:

class Fiber {
  foreign iterateStackFrames_(iterator)
  foreign moduleAt_(iterator)
  foreign functionAt_(iterator)
  foreign lineAt_(iterator)

  stackTrace {
    var trace = []
    stackTrace {|module, function, line|
       trace.append("at %( module ):%( function ) line %( line )")
    }
    return trace.join("\n")
  }

  stackTrace(cb) {
    var iterator = null
    while (iterateStackFrames_(iterator)) {
      cb.call(moduleAt_(iterator), functionAt_(iterator), lineAt_(iterator))
    }
    return cb
  }
}

[mhermier]

I have more than 250 patches)

What I wonder (long-term) is how many of those could potentially be stand-alone libraries rather than patches against Core and if that wouldn't make it far more likely for others to see and use all your hard work rather than limiting it to people who are comfortable patching and recompiling Core.

Perhaps many of your patches change internals though, I dunno...

Most of them are internal changes, rejected extensions to the languages, previous attempts of some of the features that now have a more proper pull request.

All in all, most of it is core extensions attempts (ByteArray, Char and some I don't particularly remember), and the previous attempt of the mirror API. The highest layers are attempts at solving the reentracy issue and the also an attempt at making an additional C++ version of the library (which would be almost header only).

Oh sure, of course. Just (long-term) I want to avoid doing double work. Perhaps it's more helpful to think of "binary libraries" as "optional modules that live outside the wren source tree"... that's the idea. Adding optional modules without recompiling Core.

But it should be easy to go the other direction also. Someone should be able to checkout Wren into one folder, and 5 binary libraries into other folders, and with only changing a few lines of code build a single executable with "batteries included". It's all the same thing, the only differences is whether those libraries live in 5 separate files, or in a single larger binary.

Well for me the modules is an unsolved problem, I find the builtin handling to be too much simplistic, and we should improve util/wren_to_c_string.py to a more advanced builtin resource generator with plugin stub/skeleton support, binary file support...

[joshgoebel]

more advanced builtin resource generator ... plugin stub/skeleton support

I'm not 100% sure what you mean here, perhaps you'd want to join the discussion over on wren-lang/wren-cli#52 and flesh out those ideas a bit.

we should improve util/wren_to_c_string.py

I've improved it slightly in essentials but to me tooling is almost secondary. Once we have an official binary API for dynamic loading of binary modules then we can go crazy building out the tooling to make generating those dynamic libraries much simpler. I'm not sure Wren Core is interested in these things (or if they even make sense), so mostly when I speak of these things I'm talking about Wren CLI.

Writing clean C code isn't my strong suit though so I'm hoping someone comes along interested in that piece of the puzzle. The branch I pointed you to earlier works 100%. You can compile essentials as a stand-alone dynamic library and then import it an runtime... it all "just works", but the paths are still hard-coded, etc...

[mhermier]

I mean some infrastructure/tooling to normalize external plugin loading.

I don't blame you in particular, but I think that the fork culture is plaguing wren. essentials and other solutions is not the solution for me. We lack a community page of maintained projects/libraries/resources, and contributions from the community (even if it improved a little recently).

[joshgoebel]

I mean some infrastructure/tooling to normalize external plugin loading.

That's what wren-lang/wren-cli#52 is working towards. It of course ultimately needs more testing, polish, and merging into CLI proper.

I don't blame you in particular, but I think that the fork culture is plaguing wren. essentials and other solutions is not the solution for me.

Not sure I follow. This is what I'm trying to avoid - everyone having to maintain 100 patches just to use binary libraries. essentials is just a binary library, it's not a fork of anything. In the future you'd hopefully just type wrenlib install essentials or wrenlib install json or some such --- just like you might type gem install json in Ruby.

And then the library would be installed locally and ready to use for any script.

[mhermier]

@joshgoebel Well from my understanding it, essential is a deviation based on taste/jugement. Considering the amount of possible patch maintenance work, maintaining a distribution of wren where patches would be controlled by macro configuration is not a viable solution until code stabilize for good. So I think, for now, it would be better to have a maintained list of feature branches containing only one or the fewest possible features.

[joshgoebel]

@joshgoebel Well from my understanding it, essential is a deviation based on taste/judgement.

A deviation from what? It's just a library. It would be Wren + C code that is 100% compatible with Wren Core (with zero patches). It seems very natural to me to group related functionality into modules/classes (as every other languages I know does). Some sort of "one function per branch" seems very strange (for libraries) - and also a huge amount of work vs just publishing a library - as is already common practice, even for Wren: https://github.com/wren-lang/wren/wiki/Modules.

I totally see where that makes sense for patchsets, but not libraries.

Considering the amount of possible patch maintenance work, maintaining a distribution of wren where patches would be controlled by macro configuration is not a viable solution until code stabilize for good.

I don't really have a great answer for "patches" to Wren Core... that's entirely outside the scope of what I'm working on and I think a different (but related) problem entirely...

[joshgoebel]

@mhermier Trying to get this FiberMirror stuff to work without success. Are there a bunch of other patches I'm not aware of? I get an error with the stack trace builder:

Fn does not implement 'module'.

_stackTrace.add("at %( fn.module.name ):%( fn.name ) line %( line )")

module isn't a documented getter for Fn... am I missing something? Same issue for name.

[mhermier]

Related to a patch of mine that expose modules as objects. Will think of a way to deal with that, probably using mirrors.
If you have test cases to submit, it would help me a lot.

[joshgoebel]

If you have test cases to submit, it would help me a lot.

No, but it all seems to work other than one tiny typo and missing Fn.module and Fn#name. I was porting Mirror to an external binary library just to show how it's done. :-)

probably using mirrors.

That was my thought, a FunctionMirror.

Related to a patch of mine that expose modules as objects.

Link to commit? I wasn't seeing the code.

[mhermier]

For now it will stay private. The modules are not exposed by design, and I don't want to raise more controversies for now. There is already some raging debates to fight that this one. I'll try to make it using mirrors, even if it leaks modules objects, but don't extends a class for them. (It means reflected will return a value with a null type).

[joshgoebel]

For now it will stay private. The modules are not exposed by design, and I don't want to raise more controversies for now. There is already some raging debates to fight that this one.

Hmmmm... Who is saying expose them as objects? But you have to expose at least "function name" and "module name" to have a workable stack trace - there is no way around that. So do some simply think that you shouldn't be able to retrieve a stack trace for a failing fiber?

I was imagining that you'd add:

class FunctionMirror {
  foreign name { ... }
  foreign moduleName { ... }
}

Just the bare minimum needed for stack tracing.

[mhermier]

Things are taking shape :

at ./scratch: null line 54
at mirror: null line 70

Still have some issues:

• how MethodMirror should be addressed
• probably an issue with line numbers
• reverse stack printing ordering ?

[joshgoebel]

Maybe we could coordinate a bit on Discord? I'm starting to think I may need this sooner rather than later and perhaps there is some way I could help. My context/use case is teaching others to program in Wren via programming exercises and I'm finding it really hard to work thru failing tests without a proper stack trace pointing to the line that has actually caused the error. So much so that it might actually be a blocker for my use case.

reverse stack printing ordering ?

Obviously this is the smallest detail and easy to fix. I presume people needing a stacktrace directly will be doing custom processing of it in many cases so switching the order is trivial.

[mhermier]

I don't use discord, and will not subscribe. I have a personal rule of diminishing as much as possible my internet finger print, in terms of social media/chat group/... so I only maintain what I already have, unless absolutely required. Don't know if you can send me private message on github ?

The prototype is working to some extent, thought I have a memory optimization to look before I'll push a new version for test. It seems that a function/method object is storing its name as a plain C string, and it would be more efficient to use an ObjString since we have it already in the symbol table. So I have to look what is really going on.

[joshgoebel]

Don't know if you can send me private message on github ?

Not that I know of. :) Signal?

a memory optimization to look before I'll push a new version for test.

Very curious to see what you come up with and how low-level it is. I'm still hoping to migrate the Mirror stuff to a library so it can be loaded dynamically. I was 95% there before I ran into the missing access to module/function name. :)

[mhermier]

No Signal XD

I pushed an update, I did not included the change for debug->name. While it should reduce memory usage, it didn't resulted in enough improvements for now.

[joshgoebel]

Are you sure it shouldn't be:

for (i in 0...stackFramesCount) {

With 3 dots?

it's a bit rough for anonymous functions but I guess it might be hard to find a variable name for them?

at ./test: new(_) block argument line 12
at ./test: new(_) block argument line 9
at ./test: new(_) block argument line 8
at ./test: new(_) block argument line 7

[joshgoebel]

Haven't diagnosed it yet but it also seems to get "stuck" inside classes...

at ./test: new(_) block argument line 18
at ./test: new(_) block argument line 11
at ./test: new(_) block argument line 10
at ./test: new(_) block argument line 9
at ./test: smith() line 14

import "essentials:essentials" for Strings
import "mirror" for Mirror

System.print(Strings.upcase("bob"))
System.print(Strings.downcase("LUCY"))

var e = Fn.new { Fiber.abort("bad") }
var d = Fn.new { e.call(22) }
var c = Fn.new { Boo.smith() }
var b= Fn.new { c.call(96)}
var a = Fn.new { b.call(32) }

class Boo {
    static smith() { d.call() }
}

var f = Fiber.new(Fn.new {
    a.call()
})

var x = f.try()
System.print(Mirror.reflect(f).stackTrace)
System.print("STILL RUNNING")

The stack trace just stops inside smith() when it should continue into the two other anonymous functions.

[mhermier]

Can be ... I usually don't use raw loops. But the output doesn't seems correct, I wanted to have the stackTrace call on Fiber.current

I was not able to reproduce where you are stuck.

[joshgoebel]

My fault. I was only printing the trace, not the error. I needed D not d so it could find the method. It works now:

at ./test: new(_) block argument line 18
at ./test: new(_) block argument line 11
at ./test: new(_) block argument line 10
at ./test: new(_) block argument line 9
at ./test: smith() line 14
at ./test: new(_) block argument line 8
at ./test: new(_) block argument line 7
STILL RUNNING

The new(_) for anonymous functions is just a limitation we're stuck with you think?

I wanted to have the stackTrace call on Fiber.current

I'l call it from the "main" thread but wanting the trace of the fiber that crashed. It seems to work perfectly now that I fixed the error in the code. :) (with the ... fix)

[mhermier]

I think it is a limitation of the compiler when it creates the function. For now I'm looking for functional bug, this one is more cosmetic.

[joshgoebel]

Porting into a library:

• We need a module registry to export vs a function joshgoebel/wren-essentials@8ed56e4#diff-bed23027c3ffe20e5cec93de0d8453bb70bb73bcc79981289f4c87e5ed769625R31
• We have to pull over a few things from Wren (that aren't exposed) joshgoebel/wren-essentials@8ed56e4#diff-7a77f1faae3f857b93348b56293069f98edfa75ba0b647e2ab7cf5bb87d7821fR6 (probably should live in a sep file long-term)
• The .wren code is compiled into large C string blobs and referenced in the module registry, just the same as it's done in the CLI modules. It's loaded via the same loadModules code in the CLI that's been altered to be binary library aware.

It's still a little messy since I did this by hand and I think you renamed a bunch of stuff in your last rebase and I only copied over the Fiber changes I needed.

BUT... if you're using the latest wren-essentials and my binary-libs branch you can now compile libwren_essentials.dylib and then load it into the CLI at runtime. And boom, access to stack traces via the mirror module.

So yes, it does require access to a few "wren internals" but that doesn't mean that access can't be provided at a later date via a binary extension library - it doesn't have to be compiled into the CLI from the very beginning. Just as long as you're using the same data structures, etc.... I think libraries that do this should be very careful to say so and make it clear they depend on Wren internals. Definitely not a thing every library needs to do.

---

Again this is still early and more "proof of concept"... long-term we'd hopefully want to figure out how to make this whole process easier. From a maintainer perspective what would be desirable is if I could run a single command that (at least) mostly updates the library with the source from your Core patch - but bundles it up into module library form - all without manual intervention.

I can see how someone using "Wren Core" in their own project might want a core patch for this (because what else is there to add binary functionality), but I can also see someone using Wren CLI expecting they could just install this functionality via a dynamic library, like any other binary library functionality. So it seems we need some way to keep these things in sync for libraries that would be useful in both contexts (Mirror just being one of them).

This is all assuming Mirror remains outside Core - if it were merged then this problem is solved, at least for Mirror lib.

---

What is nice to note here is the Wren code is of course already entirely portable. I simply copied it directly from your repo into mine and it works with 0 changes. All the complexity is at the C layer.

[mhermier]

Updated, I made Stacktrace a Sequenceable thought it is not yet Indexable.

[joshgoebel]

Updated, I made Stacktrace a Sequenceable thought it is not yet Indexable.

Very difficult to keep up with your rebasing. :-) That's why we need something automated. :)

[mhermier]

Because it was not meant to be taken but reviewed.

[joshgoebel]

Thanks for all your hard work on this, really do appreciate it.

Because it was not meant to be taken but reviewed.

Even for just reviewing I find it easier to keep up with moving changes to a PR when I can see the individual commits as they layer on. Perhaps it's just me.

[mhermier]

Even for just reviewing I find it easier to keep up with moving changes to a PR when I can see the individual commits as they layer on. Perhaps it's just me.

It is not just you, but a way of mine to deal with patches and propositions. I have changes that impact so much the code, that it is almost impossible to do a merge each time an update it is performed.
In addition I consider most of the patch, I issue for pull requests as "for the eyes/test only" until it was discussed, agreed. This is so I can fix patch sets, and don't have patch of patch of patch, that are impossible to revert or simply read.

[joshgoebel]

You can have your cake and eat it too if you interactively rebase. It's all about how you lay out the commits so when you rebase at the very end you can squash them down into the structure you desired all along... so while committing/developing, you have the FULL history on top (and for reviewers it's easy to see what you JUST changed), yet at the end you still have only 3 clean commits. (and they merge or patch as easily as they do now)

Git is pretty amazing like that.

[mhermier]

Yes but usually the bus factor is so long that I have to reduce the patch set often so I don't loose track of what is going on, multiplied by the number of changes that I have in flight...