Preview classes are not reloaded when using forking servers

[palkan]

Hey!

Thanks for the project, first of all.

I would like to share a potential caveat (which worth mentioning in the Readme, though not sure where—that's why an issue).

We've been using Puma in a clustered mode in development (which we prefer to catch forking related issues early), and noticed that preview classes are not being reloaded (thus, changes in the Ruby code are not reflected, we need to restart the dev server).

After some investigation, I found that Preview.clear_cache is called in the Puma master process, while web requests (which use Preview.all) are handled by workers. That leads to @previews value to never been nullified.

[allmarkedup]

@palkan thank you so much for this heads up - I've never used Puma's clustered mode so I would not have caught this! I'll certainly add a note to the readme if I can't figure out a way to solve it. It may need someone with more Ruby skills than me but I'll dig into it as soon as I can.

Really appreciate you taking the time to share this here, thanks again 👍

[xdmx]

From a quick check I'd bet that the problem is this one: https://github.com/allmarkedup/lookbook/blob/main/lib/lookbook/preview.rb#L100=

The @previews (and FWIW also the @errors below) are set at the class level, and not instance level, which is causing all sort of problems in a multi thread setting.

This could help on fixing this: https://www.rubytapas.com/2016/11/20/ruby-thread-local-variables/

[palkan]

This is not about multi-threading (though that could lead to some race conditions, which are not critical in this case), but about multi-processing.

A quick idea to overcome this is to use a file system: every time we need to reset the cache, we write some random value (or current timestamp) to tmp/cache/lookbook-digest. In the Preview.all method, we can read (and store in an instance variable) the current value (if any) and check whether it has changed to decide whether we need to reload the code.

[swanson]

We ran into this today as well. We ended up setting WEB_CONCURRENCY to 0 (to force Puma to run in single mode) and then everything started working again.

[allmarkedup]

Hey all, thanks for all the info and ideas - I'm afraid not had much time recently to spend on digging into this but I'll try to get to it soon to see if it is something that can be worked around.

@palkan - I will investigate your idea of using the file system to trigger a clearing of the cache, that definitely sounds like a possibility, thank you!

[allmarkedup]

Ok, so I've got a branch up that should hopefully fix this problem roughly using @palkan's idea above. The branch is here: https://github.com/allmarkedup/lookbook/tree/previews-concurrency-fix

@swanson @xdmx @palkan if you have any time do you think you could test this branch to see if it fixes the issue for you?

Looking into this has made it pretty clear that I need to refactor the way that the parsing/caching process works - it has grown rather organically as Lookbook has progressed and it could definitely do with being reworked. But in the meantime it would be good to get a temporary fix for this out as that refactor may not happen for a while.

[swanson]

Ok, so I've got a branch up that should hopefully fix this problem roughly using @palkan's idea above. The branch is here: https://github.com/allmarkedup/lookbook/tree/previews-concurrency-fix

@swanson @xdmx @palkan if you have any time do you think you could test this branch to see if it fixes the issue for you?

Looking into this has made it pretty clear that I need to refactor the way that the parsing/caching process works - it has grown rather organically as Lookbook has progressed and it could definitely do with being reworked. But in the meantime it would be good to get a temporary fix for this out as that refactor may not happen for a while.

Tested out the branch. With WEB_CONCURRENCY of 2 (cluster mode), the classes did seem to be reloaded correctly without restarting the server (good!), however the auto-refresh was not working. I had to manually refresh the preview, but the changes did correctly show up.

I see these log messages when changing a preview:

[ActionCable] Broadcasting to reload: {:modified=>["/Users/matt/dev/path/to/previews/button_preview.rb"], :removed=>[], :added=>[]}

With WEB_CONCURRENCY of 0 (single mode), the auto-refresh is working so I don't think it's anything related to my Lookbook or VC preview configuration.

I see these log messages when changing a preview:

[ActionCable] Broadcasting to reload: {:modified=>["/Users/matt/dev/path/to/previews/button_preview.rb"], :removed=>[], :added=>[]}
[1656976627558] Lookbook::ReloadChannel transmitting {"modified"=>["/Users/matt/dev/path/to/previews/button_preview.rbb"], "removed"=>[], "added"=>[]} (via streamed from reload)
Started GET "/lookbook/ui/button_preview/playground"

[palkan]

@allmarkedup Great!

Left a comment to the commit with some suggestions.

[allmarkedup]

Thanks so much for all your help on this @swanson @palkan and sorry progress is so slow - I've unfortunately been out with covid for a little while so I'm just catching up with myself.

I'll hopefully take another look tonight and get back to you in more detail.

[allmarkedup]

Ok so I've just pushed some updates to the previews-concurrency-fix branch based on @palkan's comments - many thanks! That should(I hope) alleviate the potential race condition. - any issues let me know.

@swanson I've not yet been able to recreate the issue you are seeing with the auto-refresh not working. However I did recently find and fix a bug where multiple ActionCable server instances were being created which is possibly the cause of the problem you are having. That fix has also been rebased into the previews-concurrency-fix branch so if you could give it (another!) spin for me and let me know how you get on I'd be eternally grateful :-)

Thanks as always for all your help 👍

[palkan]

Thanks @allmarkedup! The change looks good to me :+1

[nicolasferraro]

@allmarkedup Sorry to revive this issue, I'm setting up lookbook (1.0.0.rc.2) on the project I'm working on and had the same issue of auto-refresh not working.

It started working after setting WEB_CONCURRENCY to 0 as swanson suggested above. Has the fix been released already?

BTW, the gem feels awesome to use and looks really clean, thank you for all the hard work :)

[allmarkedup]

Hey @nicolasferraro - thanks for letting me know. The fix discussed above has already been released on both the 0.9.x and the 1.0.0-beta branches but it sounds like there are still some issues!

It's a frustrating one to try and catch because I can't seem to replicate it - I'm using puma with concurrent instances in dev and it all seems to be working for me. But clearly there is some other factor that is causing problems in certain cases.

I'll try to do some more investigation on this as soon as I can and let you know what I figure out. I don't think I'll hold up the pending 1.0.0 release on account of this however as this is still likely to be an issue in both branches currently anyway.

Sorry I can't be of more immediate help but hopefully I'll have something for you soon!

[allmarkedup]

@nicolasferraro I just wanted to check a couple of things to help me debug this. What rails version are you using? And does your config.ru file contain the line Rails.application.load_server?

Just trying to eliminate a few things to help me figure out why it's not causing an issue consistently. Thanks :-)

[nicolasferraro]

@allmarkedup Hey! sorry for the late response, we're running rails 7.0.3.1 (with ruby 3.1.0), and config.ru does include the Rails.application.load_server line.

Hopefully it helps

[allmarkedup]

@nicolasferraro ok great, thanks for the info. I've actually just managed to finally replicate the issue so at least I can now dig into it a bit more and hopefully find a proper fix.

I'll post an update here as soon as I've made any progress.

[allmarkedup]

Hey all - so I'm afraid that after looking into this more I don't think this is something that Lookbook is going to be able to support.

The problem is that under the hood Lookbook uses the Listen gem to detect file changes. I'd previously missed this note in the Listen docs:

"Listeners do not notify across forked processes, if you wish for multiple processes to receive change notifications you must listen inside of each process."

Unfortunately I don't think there is any way for me to implement that in Lookbook, and the only alternative would be for a config setting that enabled re-parsing all preview classes on each request, which I have done a spike of but is really too slow (especially on larger projects) to be of much use.

So I'm afraid the only workaround at this point is to use set WEB_CONCURRENCY to 1 when using Puma (or any other forking server).

So I'm going to close this issue now and add a note to the docs about this limitiation. Sorry that there is not a better solution!

[allmarkedup]

Hello again everyone 👋

I know this thread has been closed a while now but I just wanted to post an update here because I've just released a first beta of Lookbook v2.0, which includes a completely rewritten file watching/live updates system that (as far as I can tell so far) should fix all the concurrency/threading issues discussed above.

This was actually more by accident than by design as it was a side effect of switching to lean more heavily on Rails's own in-built class reloading/file change detection tools but it's a nice thing to get for free :-)

If anyone wants to dive in and give the beta a try then these links will hopefully be useful in getting up and running with it:

• v2.0 release discussion thread: Lookbook v2.0 beta #310
• Updated docs, upgrade guide and more: https://v2.lookbook.build (still a work in progress)
• v2.0 Lookbook UI demo and some examples of preview embeds, one of the main new features in v2.0.

And of course just give me a shout if I can help in anyway. And let me know how you get on if you are able to test it out! 🤞