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

Add servedebug(...) with Revise support #134

Closed
wants to merge 1 commit into from

Conversation

frankier
Copy link
Contributor

@frankier frankier commented Nov 7, 2023

Fixes #122

I went ahead and put this together. I know you mentioned you might work on it soon, but I had patched my own copy of Oxygen.jl to be able to have this working, so thought I may as well put together a PR.

I think it works quite nicely to have a dedicated entrypoint for a debug mode, since this will help encourage against accidentally leaving on any debug features in production. It also makes it easier to prevent using features which might interact poorly with Revise or debugging like async/parallel.

@frankier
Copy link
Contributor Author

frankier commented Nov 7, 2023

Not sure if I should update the Manifest or not if it's about to be deleted.

@ndortega
Copy link
Member

ndortega commented Nov 8, 2023

Hi @frankier,

Thanks so much for putting this together! I had a local branch where I was attempting to get this working but kept failing quite spectacularly. I haven't tried these changes out yet, but I will check them out and give it a try.

Regarding the Manifest, It's getting removed in my other PR so you can remove it in this one if you want.

@ndortega
Copy link
Member

ndortega commented Nov 8, 2023

@frankier

I'm having issues seeing changes reflected when the server is running. Could you list out the high-level steps you're doing to run the server with revise? Thanks!

@frankier
Copy link
Contributor Author

It looks like this isn't quite ready yet. I started trying to make an example, and what I noticed was that I was adding routes in my __init__ function, which would never be rerun, and so would never get revised. This hasn't been a problem in my own projects, since I've mainly been modifying library code.

I've added some extra stuff to this PR such as user Revise callbacks and some invokelatest(...) calls (the latter don't seem to help but I'm leaving them in for now) to try and help, but it still doesn't work well in all cases.

Take a look at this example: https://github.com/frankier/oxygen-revise-test

Here's what works (for me):

  1. Modifying any code called from a route handler which is not inside the route handler (normal Revise)
  2. Adding a new route in add_routes(...) (now appears to work thanks to using revise_callbacks for add_routes())

Here's what doesn't work:

  1. Modifying a @get handler defined inline will about half the time show the old version and half the time throw and error (the latter is probably when the GC has run) --- why is the new version not taking effect straight away? Things to take effect after refreshing. This seems to be regardless Is there a better way to invalidate the old ROUTER[] and kill the old server?
  2. Ditto for deleting a route

Since we still seem to have these problems, I could take a look at trying the token/queue based approach at some later date. Or perhaps this PR has given you some ideas?

@ndortega
Copy link
Member

ndortega commented Dec 2, 2023

Hi @frankier,

Thanks for the update and sorry for the slow response, but thanks for taking the time to write this all up. This has been very helpful to read through.

Going forward I'd like to spend a little more time with my current approach. I feel like I'm close, I just need to stop the server flapping issue.

@frankier
Copy link
Contributor Author

frankier commented Dec 2, 2023

Great! Ill hold off for now then.

@ndortega ndortega closed this Dec 14, 2023
@walterl walterl mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hot module reloading
2 participants