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
Try to make CI pass tests #823
Closed
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
8a2fe20
tests: skip testing that fails on 3.7, in lieu of fixing it
pythonspeed 001d602
tests: match modern Python behavior
pythonspeed d94c22e
tests: Support Python 3.11 behavior
pythonspeed d63dd13
Fix upgrading of RLocks on Python 3.11
pythonspeed 26d2382
Work with older Python 3 versions
pythonspeed 00f2e12
Fix RLock on Python 3.11
pythonspeed f39409c
Fix TLS shutdown
pythonspeed 8638e17
Fix 3.7
pythonspeed File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import sys | ||
|
||
import eventlet | ||
from eventlet import greenio, hubs, greenthread | ||
from eventlet.green import ssl | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This needs to be indented, otherwise the
style
workflow fails with: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.
From a functional perspective commenting this test seems a bit dangerous, isn't?
Without that we would release a un-acquired lock, I'm not an expert but this seems a non-sense to release something not aquired by the current process, and I don't think we want to do that.
However, as this test break py_rlock and as py_rlock is a forked buggy version of rlock, I'm not sure commenting this line will be worst than with this line un-commented.
Thoughts?
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.
Actually I agree with @4383 it seems a bit dangerous and in the case we want to have this line comment with need to add the space.
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.
is all python version impacted by this break, or simply recent version like 3.11?
May changes from https://github.com/eventlet/eventlet/pull/754/files could help to solve this problem and allow us to uncomment this condition. Especially the changes that modifying
eventlet/patcher.py
and thefix_py3_rlock
.However, to avoid pulling the entire ball of wool, we could leave this line commented for now (with the pep8 warning fixed), and, then, once merged, rewind on #754 to see if we are now able to merge it too, and, then, try to uncomment this line once all the things are aligned.
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.
Versions previous to 3.11 have a slightly different _PyRLock implementation that doesn't have this line. So previous versions could just patch
threading.RLock
withthreading._PyRLock
and it "worked".