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.

Sign up for GitHub

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

Jump to bottom

ubuntu-22.04 fix: NoneType object has no attribute getcurrent() #954

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ubuntu-22.04 fix: NoneType object has no attribute getcurrent() #954

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions eventlet/green/thread.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@


def get_ident(gr=None):
if gr is None:
return id(greenlet.getcurrent())
else:
try:
if gr is None:
return id(greenlet.getcurrent())
else:
return id(gr)
except:

Check warning on line 40 in eventlet/green/thread.py

View check run for this annotation

Codecov / codecov/patch

eventlet/green/thread.py#L40 

Added line #L40 was not covered by tests
Copy link
Member

@4383 4383 Apr 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of making id(gr) the default return value rather than try/except/if/else?

>>> gr = None
>>> id(gr)
140586328307168

Calling id(None) seems to always return something so... why not.

The idea behind these changes LGTM, I'd just suggest to refactor the logic to reduce the complexity of this function. Don't want to pick nit neither. Let me know if that sounds good for you.

https://docs.python.org/3/library/functions.html#id

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand I wonder if by forcing the return an identifier even when gr is equal to None we could may find us hidding these symptoms. Symptoms which may be the manifestation of a more pernicious problem.

Please can you give us more details about the context where you face this problem?
Do you have a reproducer?

Copy link
Member

@4383 4383 Apr 4, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if greenlet.getcurrent doesn't exist and if gr is equal to None seems weird.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's more or less the scenario you propose to follow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this eventlet module (eventlet.green.thread) is the patched version of the Python thread module I'd argue that maybe, one option, could be to return the result of the native get_ident function. It will return the ‘thread identifier’ of the current thread. IMO, That would more meaningful that returning an integer computed from a None value.

https://docs.python.org/3/library/threading.html#threading.get_ident

But I still continue to think that the observed behavior is the symptom of a problem more deeply rooted.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for your review, after searching for root of this problem, I found its not related to this package, and its about bobcat keystone package that uses this package. and I tried to fix actual problem and eventlet package works fine.

for reproduce this package install openstack prereqs and set bobcat, then install keystone, and run su -s /bin/sh -c "keystone-manage db_sync", in ubuntu 22.04, you can see error.
thank you again 🙌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your feedback. So, if you agree with abandoning this proposal I think we can close this github pull request and its associated github issue. Do you agree?

return id(gr)


Expand Down