Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Sep 15, 2021

On master (2161a05), running functional tests that use the P2P interface ends with an error:

RuntimeError: Event loop is closed

This PR fixes this bug, and enables more functional tests on Windows MSVC CI task.

More details about bugfix:

Excluded tests, that are listed in the EXCLUDE_TESTS environment variable, need more thorough investigation to be enabled.

@fanquake
Copy link
Member

In master we still require a minimum Python version of 3.6. From what I can see, WindowsSelectorEventLoopPolicy didn't exist until Python 3.8.

@hebasto
Copy link
Member Author

hebasto commented Sep 16, 2021

In master we still require a minimum Python version of 3.6. From what I can see, WindowsSelectorEventLoopPolicy didn't exist until Python 3.8.

Right. Would it be sufficient to update our docs and specifically for Windows users mention that they are required to install Python 3.8+ ?

Or add a version check?

@maflcko
Copy link
Member

maflcko commented Sep 16, 2021

This should be fine. Python doesn't care about unexecuted code.

I am running python3.6 on my machine with the following diff and everything passed:

diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index ec563cc290..1cf23ef191 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -578,7 +578,7 @@ class NetworkThread(threading.Thread):
         NetworkThread.listeners = {}
         NetworkThread.protos = {}
         if sys.platform == 'win32':
-            asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
+            asyncio.foo_bar(asyncio.FooBar())
         NetworkThread.network_event_loop = asyncio.new_event_loop()
 
     def run(self):

Again, I don't think anyone of our users is running the functional tests on windows, so it seems odd to even think about spending time to support running them on older versions of python.

@maflcko
Copy link
Member

maflcko commented Sep 16, 2021

review ACK 357f0c7 🌆

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 357f0c723308b296c6250946c26cf476d9ecfda2 🌆
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgHSAwAjiVz1b6sTwNnO3B+lE+GAE8Uza3wmZE0U6wzpHAyiFlN3IbN9nDIMXHF
XhI5v36mNT5OgrBtR7KlRbbEuhPLpz145KZkuWvbGfqo7IqIUOOrttgmmAqSnMyb
O/ksg37qt7pE9bQwwdqNUNVDpy6el0ijAOVX+M4GW9JStz8LxgJMG9qPxv/1ZQzW
u2x5GCpUrxvJ7DjufwK/6+Z0WN2I0G8/c0M3+56xHrB5Y3yCm3K1Dm4olZty2GBh
a0vpeVrKywKyDmLS1SQp4NdNPnAes2LYI4zijleYFaQYxGTgi/tQ2Dz8WCKrqIMK
4VGbyNTIy20EsQCHNkpuv168dyETSTyJ8wFUQs11x1kRcCeM5inWbrWwNXuOK/sO
BvQcQ0qHKM+86iW/xL1+VgWS/SNQ4DrUBo4BO0NecosxuNYDCFYi1eWKwOUVSNDV
UPhN0yB/jP/b/xIxXXPVOgw5Agn5psrVqLB+JYE+mWHDTF5aoUswFdwBrQilzksT
EnyWZnM4
=U9D7
-----END PGP SIGNATURE-----

Timestamp of file with hash 81344abdd7d21141fc132bf636bb4c3a9b6e24b5ef0ed9a189290709b209a0d8 -

@maflcko
Copy link
Member

maflcko commented Sep 17, 2021

I reset the ci run 4 times, all times it did pass.

Would be good to merge this before the std::fs refactor, so that it has better test coverage.

@fanquake
Copy link
Member

This should be fine. Python doesn't care about unexecuted code.

Sure. However I do care about implicit / undocumented dependency changes. At a minimum the commit / PR description should mention the change in requirements.

@maflcko
Copy link
Member

maflcko commented Sep 18, 2021

The PR description links to "what is new in Python 3.7". Is there anything else it can say?

@fanquake
Copy link
Member

fanquake commented Sep 18, 2021

Is there anything else it can say?

Link to / mention the actual minimum requirement, which is 3.8?

@hebasto
Copy link
Member Author

hebasto commented Sep 18, 2021

Is there anything else it can say?

Link to / mention the actual minimum requirement, which is 3.8?

Added.

fanquake added a commit that referenced this pull request Sep 18, 2021
357f0c7 ci: Enable more functional tests on Windows MSVC task (Hennadii Stepanov)
f559326 qa: Fix "RuntimeError: Event loop is closed" on Windows (Hennadii Stepanov)

Pull request description:

  On master (2161a05), running functional tests that use the P2P interface ends with an error:
  ```
  RuntimeError: Event loop is closed
  ```

  This PR fixes this bug, and enables more functional tests on Windows MSVC CI task.

  More details about bugfix:
  - [What’s New In Python 3.7](https://docs.python.org/3/whatsnew/3.7.html#asyncio)
  - https://bugs.python.org/issue33792
  - actual [change](https://docs.python.org/3.8/library/asyncio-policy.html#asyncio.WindowsSelectorEventLoopPolicy) done in Python 3.8

  Excluded tests, that are listed in the `EXCLUDE_TESTS` environment variable, need more thorough investigation to be enabled.

ACKs for top commit:
  MarcoFalke:
    review ACK 357f0c7 🌆

Tree-SHA512: d0ba85be81d55c934959ce7402a9c726598125e9751a1de179d16759d0e8b8a915de879c3a62c12d3564c5e0d9649ebd86963744449626efaa42d9eaa99ad3d0
@fanquake
Copy link
Member

This has been merged.

@fanquake fanquake closed this Sep 18, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
@hebasto hebasto deleted the 210915-loop branch September 24, 2021 16:24
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants