-
-
Notifications
You must be signed in to change notification settings - Fork 335
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 curio-inspired POC monitor #429
Conversation
🍰 🎉 Not a code review, but some random thoughts: Check out The hazmat docs have an example of getting a traceback from a non-running task, so you can make that Just Work. (Maybe we should provide a helper on the Even if you want to keep the monitor in a separate thread, it might be convenient to use trio in that thread instead of having to implement your own cancellation support. (You can have multiple calls to Though it might make just as much sense to drop the threads altogether and run the monitor as a regular task. You have to get into the main thread to do anything interesting anyway, and it would be nice if the monitor could run on an arbitrary |
Yup, that would be really convenient for debugging
First it's not my own cancellation support, but Curio's one 😆
But this means the monitor won't respond if the trio loop is blocked, which is a typical usecase where we would like to use the monitor in the first place
We can do all the read-only stuff from another thread, this seems a good part to me
\^__^/ |
Another thing I'm not sure is the |
IMO whilst this sounds (and is) cool, you open yourself up to the Werkzeug debugger attack if you listen on a remotely accessible socket, so you'd either have to tell users not to do that, add built-in authentication of some form, or allow users to add their own at which point you'd be making an even more complex debugger framework which seems out-of-scope for a basic monitor. |
Oh, I see, I missed that you were using an The other approach is to wait until someone actually connects to the monitor before doing anything. This is nice because it doesn't add any overhead, so you can leave your production app running in this mode and then connect after things have gone wrong. The downside is that if things have really gone wrong, you might not be able to connect...
Oh for sure you don't want to allow untrusted connections to a debug interface like this. But "trusted" and "local" aren't necessarily the same. You could have a policy that even local users need to provide some authentication before connecting to the debug interface. Or you could be in a situation where your program is running in a container environment and you have to export a regular tcp socket to access it at all (presumably locking it down somehow in the process). Or maybe you really want to access it remotely and are taking appropriate precautions, like requiring an SSL client certificate or ssh authentication. (The twisted "manhole" interface supports a mode where you connect using ssh.) If the core interface just takes a Thinking about it, we might be able to make the task tree dumping work from other threads, without actually entering the trio thread. Most operations would be to be run on the thread though. So maybe a best-of-all-worlds approach would be to make the monitor use trio for power/flexibility, and design it so that it can run either in the main thread or in a secondary thread. This shouldn't be terribly difficult; basically it means routing operations through a portal object, which could either send them over to the main thread or else run them directly. And for a few operations that we particularly want to work even if the main thread is stuck, we can do some special case handling. This way it'd even be possible to detect when our attempt to enter the main thread had gotten stuck, and print a stack dump. |
One reason I choose to use a shadow table for the tasks was to be able to keep track of their state. I didn't see a way to retrieve this information from the task object itself. |
Which state are you unable to get from the task object? |
Codecov Report
@@ Coverage Diff @@
## master #429 +/- ##
==========================================
- Coverage 99.2% 97.67% -1.53%
==========================================
Files 89 90 +1
Lines 10332 10729 +397
Branches 719 773 +54
==========================================
+ Hits 10250 10480 +230
- Misses 63 225 +162
- Partials 19 24 +5
Continue to review full report at Codecov.
|
I've corrected the stacktrace print system following @njsmith advices Finally I've copied the |
I wanted to test this out, but it seems to error?
EDIT: I guess passing this as It also doesn't work at all if you add it after calling |
Also, I got some errors when doing
|
I've only tried it by passing to the
I saw this error when trying to get the stacktrace of the |
@touilleMan @Fuyukai I'd like to close this and encourage you both to work on trio-monitor, but there's no code there yet :-). How do you want to move forward? |
This is super cool work, but I don't think this specific PR is going anywhere, both because it should probably move to trio-monitor or similar, and because empirically nothing has happened in ~6 months :-). So I'm going to close this to get it off the list of open PRs. If you have time to pick it up again at some point though then that would be awesome. |
@njsmith We talk about this PR last weekend at the europython with @Fuyukai However I don't know how far @Fuyukai with this during the sprint (I was working on triopg on my side) |
@touilleMan awesome, thanks for the update! |
Following #413, this is basically a port of the Curio monitor system to trio. I've been pleased by the instrument system which made the task really simple ;-)
I don't believe we should currently merge this PR, it's just to keep a track and start playing with something ;-)
As @njsmith said, this is probably better to move this into it own pypi library once it will mature enough