Skip to content
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

Timing issues with aside sections #86

Closed
cedriessen opened this issue Aug 23, 2022 · 8 comments
Closed

Timing issues with aside sections #86

cedriessen opened this issue Aug 23, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@cedriessen
Copy link

First: Great library, I like it a lot!

Describe the bug

That very simple demo below always only yields

Do something...

swallowing all the asides.

session {
  section {
    textLine("Do something...")
  }.run {
    aside { textLine(" - a") }
    aside { textLine(" - b") }
    aside { textLine(" - b") }
    aside { textLine(" - f") }
    // to be replaced
  }
}

If I replace // to be replaced at the end of the run block with something like Thread.sleep(1) (yes 1 ms is enough) or a "heavy" computation that takes some ms everything's printed out. I am suspecting a timing issue.

Do you have any thoughts?

@cedriessen cedriessen added the bug Something isn't working label Aug 23, 2022
@bitspittle
Copy link
Contributor

bitspittle commented Oct 1, 2022

Hey, I am so sorry for the long delay in replying. I missed the notification and just saw it now! Especially, I really appreciate your quick example to repro.

For now, to avoid using a sleep, I would recommend solving this using runUntilSignal:

session {
  section {
    textLine("Do something...")
  }.runUntilSignal {
    aside { textLine(" - a") }
    aside { textLine(" - b") }
    aside { textLine(" - b") }
    aside { textLine(" - f") }
    signal()
  }
}

Note that you're running into a threading / race issue. When I ran this on my machine, one or two of the asides would actually print.

Beyond that, I will investiagate. I suspect what should happen is the run block shouldn't consider itself done as long as there are any remaining aside blocks queued up for rendering. It's been a while since I looked at this code though and it might be a hairy, subtle interaction...

@bitspittle
Copy link
Contributor

bitspittle commented Oct 1, 2022

OK, looked a little more. What's happening is that a section is responsible for processing any aside blocks enqueued by the run method.

But your section is ending so fast, it shuts down and then the run block continues to add more aside blocks. (section and run are two different threads working at the same time)

I'm experimenting to see if it's trivial enough to detect, when the run method finishes, if one or more asides are still left, at which point, we kick off one more render request for the section (which should print out those remaining asides).

But so far the code is fighting me a bit (because debugging subtle threading issues is hard)...

@cedriessen
Copy link
Author

Thanks for looking into it and providing me with a solution.

@bitspittle
Copy link
Contributor

You're welcome. BTW I'm planning on looking at this again today.

After getting stuck last time, I decided to detour a little. I ended up finally starting to write unit tests (something I've been meaning to do for Kotter a long time -- I always wanted good code coverage to be a requirement to tag this library as v1.0).

What pushed me over the edge was the motivation to come back to this very bug. I'm hoping I can write a test that I can quickly run hundreds/thousands of times, to make sure things are working as expected.

@bitspittle
Copy link
Contributor

bitspittle commented Oct 8, 2022

OK, got it. It wasn't really that hard to fix once I got familiar with the code again. This was a great bug -- thanks for reporting!

I also added a unit test for this issue, and ran it a lot. It seems to be a solid fix.

This will be fixed whenever 0.9.10 (or later) goes out. The signal fix above is harmless to keep at that point, but you won't need it anymore.

@cedriessen
Copy link
Author

Ok. As mentioned on the other issue I will give it a try. Thanks for investing your time helping me out with that.

@bitspittle
Copy link
Contributor

bitspittle commented Oct 9, 2022 via email

@cedriessen
Copy link
Author

Just wanted to let you know: I've upgraded to 1.0.0-rc1 and things work like a charm :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants