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

Thread safety of quickjs #206

Closed
llewellyn-marriott opened this issue Feb 12, 2024 · 5 comments
Closed

Thread safety of quickjs #206

llewellyn-marriott opened this issue Feb 12, 2024 · 5 comments

Comments

@llewellyn-marriott
Copy link

llewellyn-marriott commented Feb 12, 2024

I have been having issues with PX crashing inconsistently, the logs wouldn't show much, until today when I noticed the following error:

MainProcess: Thread_33: 1707706381: /do_CONNECT/do_curl/select: 84b6c97c92f2a8d3c6adea7983b447d95e5cc656: Starting select loop
  File "C:\PX\PX-V08~1.4-W\http\server.py", line 421, in handle_one_request
  File "C:\PX\PX-V08~1.4-W\px\main.py", line 396, in do_CONNECT
  File "C:\PX\PX-V08~1.4-W\px\main.py", line 329, in do_curl
  File "C:\PX\PX-V08~1.4-W\px\main.py", line 403, in get_destination
  File "C:\PX\PX-V08~1.4-W\px\wproxy.py", line 467, in find_proxy_for_url
  File "C:\PX\PX-V08~1.4-W\px\wproxy.py", line 258, in find_proxy_for_url
  File "C:\PX\PX-V08~1.4-W\px\pac.py", line 71, in find_proxy_for_url
_quickjs.JSException: (Failed obtaining QuickJS error string. Concurrency issue?)

Note on the version number: I did try updating to 0.9 but still had intermittent crashes, with varying errors. I just don't have any of them saved.

I had a look through the code (I am not a Python dev) and from what I can see there is no state lock used when calling quickjs, which is shared between multiple threads.

In my px.ini file I set threads = 1 and the intermittent crashes went away.

Looking at the quickjs Python package (https://pypi.org/project/quickjs/), it indicates that:

QuickJS is currently thread-hostile, so this wrapper makes sure that all calls to the same JS runtime comes from the same thread.

Also, thank you for all your hard work on px, it has really been very useful for me.

@awl1
Copy link

awl1 commented Feb 12, 2024

Suffering from the same issue here - here is a screenshot of such a quickjs crash:

quickjs_crash

Reproducing is easy: Just use Eclipse IDE, configure px 0.9.0 (localhost:3128) as manual proxy and try to install any more complex plugin from the marketplace -> Eclipse tries to access px.exe from multiple threads, leading to the assertion failure...

I fully agree: Thank you for all your hard work on px, it has really been very useful for everybody here at my customer behind the NTLM/Kerberos proxy...

@genotrance
Copy link
Owner

Thank you for the report - I'll get this resolved in v0.9.1.

@genotrance
Copy link
Owner

Fixed in v091. Thank you for making it so easy @llewellyn-marriott!

The quickjs Python library is thread-safe if Function is used. If the Context class is used directly, it can only ever be accessed by the same thread. This is true even if the accesses are not concurrent.

@genotrance
Copy link
Owner

v0.9.1 has been released.

@mckirk
Copy link

mckirk commented Oct 14, 2024

The quickjs Python library is thread-safe if Function is used. If the Context class is used directly, it can only ever be accessed by the same thread. This is true even if the accesses are not concurrent.

Sorry to resurrect a closed issue @genotrance, but reading this makes me wonder if the fix actually goes far enough. The quoted text seems to imply that a Context can only ever be accessed by the thread that created it, whereas the fix (as I understand it) only prevents concurrent accesses by different threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants