Fix Windows NotImplementedError / Hanging of program #147
Conversation
Apply Sweep Rules to your PR?
|
|
@ziloka awesome. |
ziloka
force-pushed
the
fix/NotImplementedError
branch
from
f96a938
to
272a9cd
Compare
|
Relate to #113 |
|
I need to add a fix . I don't think this works just yet, sorry |
ziloka
force-pushed
the
fix/NotImplementedError
branch
from
95ba144
to
63ae7f8
Compare
ziloka
force-pushed
the
fix/NotImplementedError
branch
from
63ae7f8
to
dbd9fb9
Compare
|
The program stops hanging but it is slow. Python 3.11 and python 3.12 will work but it will have (deprecation?) warnings about Transport Sockets. Note that I only tested the find command |
|
I am unable to convert this into a "ready" pull request |
|
We might want to look into this issue |
|
|
This pull request does not resolve the serve issue. To fix that issue, I believe it is necessary to migrate from asyncio to trio. In their code. I would like to know the other maintainer's thoughts before making a merge for this. |
WalkthroughThe overarching modification across the updated files involves enhancing the handling and initialization of the asyncio event loop, particularly focusing on compatibility with Windows systems. This is achieved by explicitly importing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (14)
- examples/basic.py (2 hunks)
- examples/find_and_save.py (2 hunks)
- examples/find_and_use.py (3 hunks)
- examples/only_grab.py (2 hunks)
- examples/proxy_server.py (2 hunks)
- examples/proxy_smtp_port.py (3 hunks)
- examples/use_existing_proxy.py (2 hunks)
- proxybroker/api.py (1 hunks)
- proxybroker/checker.py (1 hunks)
- proxybroker/cli.py (1 hunks)
- proxybroker/judge.py (1 hunks)
- proxybroker/providers.py (1 hunks)
- proxybroker/resolver.py (1 hunks)
- proxybroker/server.py (2 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional comments: 32
examples/basic.py (4)
- 3-3: Importing
sysis necessary for checking the platform. This change is appropriate for the intended compatibility improvements.- 16-17: Setting the event loop policy for Windows is a critical change for compatibility with Python 3.11.0 on Windows platforms. This ensures that the
WindowsSelectorEventLoopPolicyis used, which is necessary for the program to function correctly on Windows.- 19-20: Creating and setting a new event loop is essential for ensuring that the asyncio operations use the correct event loop, especially after setting the event loop policy for Windows. This change is consistent with the goal of enhancing compatibility.
- 23-23: Passing the newly created event loop to the
Brokerconstructor is a necessary step to ensure that all asynchronous operations within theBrokeruse the correct event loop. This change aligns with the modifications made for event loop management.examples/only_grab.py (4)
- 4-4: The import of
sysis necessary for platform checking, which is crucial for the compatibility improvements aimed at Windows platforms.- 9-10: Setting the event loop policy for Windows with
WindowsSelectorEventLoopPolicyis essential for compatibility with Python 3.11.0 on Windows. This change is consistent with the overall goal of the PR.- 23-24: Creating and setting a new event loop ensures that the asyncio operations use the correct event loop, especially important after setting the event loop policy for Windows. This is a necessary change for the intended compatibility improvements.
- 27-27: Passing the newly created event loop to the
Brokerconstructor ensures that all asynchronous operations within theBrokeruse the correct event loop. This change is consistent with the modifications made for event loop management.examples/find_and_save.py (4)
- 3-3: Importing
sysfor platform checking is appropriate and necessary for the compatibility improvements with Windows platforms.- 8-9: Setting the event loop policy for Windows is crucial for ensuring compatibility with Python 3.11.0 on Windows. This change is aligned with the PR's objectives.
- 24-25: Creating and setting a new event loop is essential for the correct functioning of asyncio operations, especially after setting the event loop policy for Windows. This change supports the intended compatibility improvements.
- 27-27: Passing the newly created event loop to the
Brokerconstructor is necessary to ensure that all asynchronous operations within theBrokeruse the correct event loop. This is consistent with the event loop management modifications.examples/proxy_smtp_port.py (4)
- 4-4: Importing
sysfor platform checking is necessary for the compatibility improvements aimed at Windows platforms. This change is appropriate.- 9-10: Setting the event loop policy for Windows with
WindowsSelectorEventLoopPolicyis essential for compatibility with Python 3.11.0 on Windows. This change is consistent with the PR's objectives.- 23-24: Creating and setting a new event loop ensures that the asyncio operations use the correct event loop, especially important after setting the event loop policy for Windows. This is a necessary change for the intended compatibility improvements.
- 26-26: Passing the newly created event loop to the
Brokerconstructor ensures that all asynchronous operations within theBrokeruse the correct event loop. This change is consistent with the modifications made for event loop management.examples/use_existing_proxy.py (3)
- 4-4: The import of
sysis necessary for platform checking, which is crucial for the compatibility improvements aimed at Windows platforms.- 9-10: Setting the event loop policy for Windows with
WindowsSelectorEventLoopPolicyis essential for compatibility with Python 3.11.0 on Windows. This change is consistent with the overall goal of the PR.- 39-40: Creating and setting a new event loop ensures that the asyncio operations use the correct event loop, especially important after setting the event loop policy for Windows. This is a necessary change for the intended compatibility improvements.
examples/proxy_server.py (3)
- 4-4: Importing
sysfor platform checking is necessary for the compatibility improvements aimed at Windows platforms. This change is appropriate.- 11-12: Setting the event loop policy for Windows with
WindowsSelectorEventLoopPolicyis essential for compatibility with Python 3.11.0 on Windows. This change is consistent with the PR's objectives.- 41-42: Creating and setting a new event loop ensures that the asyncio operations use the correct event loop, especially important after setting the event loop policy for Windows. This is a necessary change for the intended compatibility improvements.
examples/find_and_use.py (3)
- 7-7: The import of
sysis necessary for platform checking, which is crucial for the compatibility improvements aimed at Windows platforms.- 16-17: Setting the event loop policy for Windows with
WindowsSelectorEventLoopPolicyis essential for compatibility with Python 3.11.0 on Windows. This change is consistent with the overall goal of the PR.- 53-54: Creating and setting a new event loop ensures that the asyncio operations use the correct event loop, especially important after setting the event loop policy for Windows. This is a necessary change for the intended compatibility improvements.
proxybroker/resolver.py (1)
- 47-47: The modification to explicitly set the
_loopattribute from the passedloopparameter, without defaulting toasyncio.get_event_loop(), is a positive change. It aligns with the PR's objective to manage event loops more explicitly, which is crucial for compatibility with Windows and newer Python versions. This approach ensures that theResolverclass uses the event loop that is explicitly passed to it, providing a more controlled way of handling event loops across the application.proxybroker/checker.py (1)
- 46-46: The modification to explicitly set the
_loopattribute from the passedloopparameter, without defaulting toasyncio.get_event_loop(), is consistent with the changes in other parts of the application and aligns with the PR's objective to manage event loops more explicitly. This approach ensures that theCheckerclass uses the event loop that is explicitly passed to it, contributing to a more controlled and consistent way of handling event loops across the application, especially important for compatibility with Windows and newer Python versions.proxybroker/cli.py (1)
- 390-391: The changes to explicitly create and set the event loop using
asyncio.new_event_loop()andasyncio.set_event_loop(loop)in theclifunction are positive and align with the PR's objective to manage event loops more explicitly. This approach ensures that theclifunction uses a specific event loop that is compatible with the rest of the application's event loop management strategy, which is crucial for compatibility with Windows and newer Python versions.proxybroker/server.py (2)
- 134-134: The modification to explicitly set the
_loopattribute from the passedloopparameter, without defaulting toasyncio.get_event_loop(), in theServerclass is a positive change. It aligns with the PR's objective to manage event loops more explicitly, ensuring that theServerclass uses the event loop that is explicitly passed to it. This approach contributes to a more controlled and consistent way of handling event loops across the application, especially important for compatibility with Windows and newer Python versions.- 154-154: Removing the
loopparameter from thestart_servercall in thestartmethod of theServerclass aligns with the changes made to explicitly manage event loops within the application. This ensures consistency in how event loops are handled and is crucial for compatibility with Windows and newer Python versions.proxybroker/providers.py (2)
- 48-48: Given the change to explicitly require an event loop when instantiating
Provider, it's crucial to thoroughly test and validate the impact on asynchronous operations throughout the codebase. This includes ensuring that the event loop is correctly passed and managed in all asynchronous contexts whereProvideror its subclasses are used.Ensure comprehensive testing, especially on Windows platforms with Python 3.11.0, to verify that the change does not introduce regressions or negatively affect performance. Consider adding unit tests that cover various scenarios of event loop management and usage.
- 48-48: To support users in adapting to the change in event loop management, consider enhancing documentation to clearly explain the need to explicitly pass an event loop when using
Provider. Additionally, robust error handling for cases where an event loop is not provided can help prevent common issues and improve the user experience. Finally, consider backward compatibility strategies to ease the transition for existing users of the library.
- Enhance documentation to cover the change in event loop management.
- Implement robust error handling for missing event loop arguments.
- Consider backward compatibility strategies to support existing users.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- proxybroker/api.py (1 hunks)
Additional comments: 1
proxybroker/api.py (1)
- 67-67: The update to the
_loopvariable initialization using conditional assignment is a good practice for managing event loops in asyncio-based applications. It ensures flexibility and compatibility across different environments.However, it's important to verify the impact of this change on the overall functionality and compatibility of the ProxyBroker program, especially considering the PR's objectives to address compatibility issues with newer Python versions on Windows.
Verification successful
The verification process has confirmed that the change to the
_loopvariable initialization in theBrokerclass is well-integrated across the codebase. The instances of theBrokerclass being instantiated with an explicitly passedloopparameter in various parts of the codebase align with the updated logic to allow for a default event loop when none is provided. This ensures flexibility and compatibility across different environments, aligning with the objectives of the PR to address compatibility issues with newer Python versions on Windows.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the Broker class is correctly utilized across the codebase with the updated event loop initialization logic. rg --type py "Broker(" # Ensure that there are no direct calls to asyncio.get_event_loop() that might conflict with the Broker class's event loop management. rg --type py "asyncio.get_event_loop()" --context 2Length of output: 1046
Script:
#!/bin/bash # Correct the regex error and verify the Broker class is correctly utilized across the codebase with the updated event loop initialization logic. rg --type py "Broker\(" # No need to rerun the search for asyncio.get_event_loop() as it has already been successfully executed.Length of output: 477
The ProxyBroker program will hang when the find command (at least, the only thing tested) is executed.
This PR allows proxybroker to be used on python 3.11.0 and returns some output.
3.12.0 does not work due to not being able to compile aiohttp not supporting 3.12
Summary by CodeRabbit