-
Notifications
You must be signed in to change notification settings - Fork 57
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
Minimal changes to allow manual re-initialization of NodeJS process #131
Conversation
@@ -163,7 +164,7 @@ def loop(self): | |||
self.threads = [x for x in self.threads if x[2].is_alive()] | |||
|
|||
if len(self.freeable) > 40: | |||
self.queue_payload({"r": r, "action": "free", "ffid": "", "args": self.freeable}) | |||
self.queue_payload({"r": 0, "action": "free", "ffid": "", "args": self.freeable}) |
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.
About this change: currently this does not work at all because r
is not defined. As this calls JS without listening to a return, the request id is ignored so passing "r": 0
works.
Thanks for working on this. As the lib was not designed around the Node.js process being stopped, all variables and all state depending on the previous node process will stop working and throw errors. Not just globalThis, but require(), and any other function currently stored on the Python side depending on a reference in JS land. This may or may not be handled by the user but if they intend to use this new API call, I think it's OK to assume the user will have written their code in a way to handle this (re-doing imports, using However I don't understand the removal of the error handling when the Node process is killed or terminated. This is intended to prevent later errors and other undefined behavior. If the Node process crashes this normally means the exception happened outside of a call done by Python as we handle most errors on object doing property accesses and calls. Can you explain this change? |
Thank you for the review! I have initially removed the error handling because that was inadvertently terminating the new Node process. However I took a second look at it and figured out a better way to handle this by checking the current Node process status and adding calls to |
Maybe this could be used in a mechanism to restart NodeJS in case it crashes or the JS app there ends up in an invalid state? I don't think I need multiple instances, but am using the bridge in a long running Python server, to use a JS/TS lib that does websocket calls to a service. We haven't had problems, but was just thinking that maybe something like this could help if we get a need to restart the bridge / node some day. I will be needing multiple worker processes though but I think those end up with separate Python interpreters and thus bridge instances & node processes anyhow. |
@@ -20,6 +27,11 @@ def init(): | |||
init() | |||
|
|||
|
|||
def terminate(): |
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.
Can you add a note for this and the other new API method to the docs along with an explanation or how to use?
Yes, this is exactly the use case for this change. Note that this does not add support for parallel NodeJS process, but it allows the process to be re-initialized. With this change, it is now possible to wrap every (independent) JS call with |
@extremeheat I have just added two paragraphs in |
Thanks for the contribution! I agree with the comment about improving error handling but that can be handled outside the PR and may require some additional thought (as we'd have the same problem of existing state being unrecoverable, so something would need to be done to deal with this). |
This PR aims to solve issue #130 by creating an interface to terminate and initialize the bridge's underlying NodeJS process and threads responsible for the communication.
To do this, I moved the calls responsible for referencing the bridge classes instances to the initialization function, refactored
EventLoop
andEventExecutorThread
's constructors to re-initialize its properties, changed the connection logic so it does not stop upon exception or process termination, and implemented aterminate()
function in__init__.py
.With this change, the following behaviour is possible:
Upon running this code, each time we
terminate()
andinit()
, a new NodeJS process is spawned and the bridge switches to communicate with it. Note that the firsteval_js
call can be done without callinginit()
because this function is called within the module's initialization. The function itself checks if the bridge has been previously initialized, so calling it again to ensure it is up is fine.Most importantly, these changes should not break the current usage of the bridge. I have verified this by running
test.py
andtest_general.py
, and no exception is raised.I have only noticed one caveat about these changes regarding the global variables exposed by the module. When running the bridge interface after re-initializing the NodeJS process at least once, the following code does NOT work:
However, this DOES work:
This is because the variable we get from
from javascript import globalThis
can only point to the global context of the first process as that's the reference that exists during the module import. On the other hand, callingjavascript.globalThis
can return the updated reference after the bridge re-initializes. I suspect this is also the case forconsole
andRegExp
as they are exposed with the same logic.A cleaner way to solve this would be to write a getter for each of these variables and expose that instead of the references to variables themselves. For example, we can write in
__init__.py
:This however would change the interface to the bridge (i.e., we would have to call
get_global_this().Date()
in the code above), therefore I did not commit it in this PR. Please let me know if this is desirable though.