-
Notifications
You must be signed in to change notification settings - Fork 81
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 remote python console based on named pipes #1009
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool idea, and I'm glad someone else is brave enough to play with messages on threads 😛
@@ -1389,6 +1390,12 @@ bool plClient::StartInit() | |||
fConsole->RegisterAs( kConsoleObject_KEY ); // fixedKey from plFixedKey.h | |||
fConsole->Init( fConsoleEngine ); | |||
|
|||
#ifndef PLASMA_EXTERNAL_RELEASE | |||
fRemoteConsole = new pfRemoteConsole(); | |||
fRemoteConsole->RegisterAs( kRemoteConsoleObject_KEY ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like we ever need to address messages to the RemoteConsole by its key, so we probably don't need a fixed key entry for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You still need a key to receive messages though, so this is not really incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly copy pasted from the fConsole code above. I used to not have the RegisterAs
call, but the messages wouldn't ever arrive to the console. Is there another way to register the plRemoteConsole
without a fixed key?
return script.contains("\n"); | ||
} | ||
|
||
void handlePipeServer() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the person trying to get this engine running on macOS & Linux, I'd really love a cross platform way of doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me wonders if pnAsyncCore should learn about named pipes, then you get callbacks on the main thread for free. Then this silly worker thread and the synchronization questions go away. Maybe @zrax will have some input on this, considering the asio work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I actually gave some thought to this while writing the code. It should actually be fairly trivial to make the code cross-platform, by making a very lightweight abstraction layer between named pipes and unix sockets on linux (and you'd connect to the unix socket with socat or something when on macOS/Linux).
Regarding pnAsyncCore, I actually started by trying to add pipe support there, but was a bit daunted by how complex it looked, and since I wanted to have a quick proof of concept runnig, I quickly changed to the worker thread approach. I do agree that it'd probably be cleaner that way though. I'll try to look into it when I've fixed the remaining TODOs.
pfRemoteConsoleMsg *cMsg = new pfRemoteConsoleMsg(curScript); | ||
std::shared_ptr<pfRemoteConsoleMsgOutput> output(cMsg->GetOutput()); | ||
|
||
cMsg->Send(nullptr, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the implementation of plDispatch::IMsgEnqueue
this should actually be safe from to call from a thread, as long as the second parameter is true
.
It will lock a mutex before adding it to the queue of messages to be dispatched.
If the second parameter is false, it will also immediately try to dispatch it and probably call the receiver's MsgReceive
handler on the thread which is probably not safe.
This allows easily debugging python by connecting with PuTTY to a named pipe.
How to use:
\\.\pipe\URU-PYTHON-<PID>
(For instance, if URU is running with pid 5151, connect to\\.\pipe\URU-PYTHON-5151
).How it works:
The
pfRemoteConsole
class is responsible for setting everything up. It essentially creates a thread that handles the named pipe (connecting it, reading from it to get a script to run, and writing the output of the script back). When it gets a python script to run, it sends a message back to the main thread by dispatching apfRemoteConsoleMsg
. ThepfRemoteConsole::MsgReceive
will then take care of executing this message, and signaling the thread back when the output is acquired. This ensures that the python logic executes on the main thread, which should avoid data races.Note that I'm not 100% sure this design is sound. Mainly, is it OK to call
plMessage::Send
from a new thread?TODO: