genotrance / px

An HTTP proxy server to automatically authenticate through an NTLM proxy
MIT License
955 stars 99 forks source link

Thread safety of quickjs #206

Closed llewellyn-marriott closed 8 months ago

llewellyn-marriott commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

genotrance commented 8 months ago

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 commented 8 months ago

v0.9.1 has been released.

mckirk commented 1 month ago

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.