Skip to content

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

Open
farshan-dev wants to merge 1 commit into
eventlet:masterfrom
farshan-dev:fix-953
Open

farshan-dev wants to merge 1 commit into
eventlet:masterfrom
farshan-dev:fix-953

Conversation

@farshan-dev

Copy link
Copy Markdown

related issue: #953

Comment thread eventlet/green/thread.py
return id(greenlet.getcurrent())
else:
return id(gr)
except:

@4383 4383 Apr 4, 2024 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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?

@4383 4383 Apr 4, 2024 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@codecov

codecov Bot commented Apr 4, 2024 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 56%. Comparing base (c22b896) to head (62f1e79).
⚠️ Report is 62 commits behind head on master.

Files with missing lines Patch % Lines
eventlet/green/thread.py 80% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##           master   #954   +/-   ##
=====================================
- Coverage      56%    56%   -1%     
=====================================
  Files          89     89           
  Lines        9767   9770    +3     
  Branches     1819   1819           
=====================================
- Hits         5485   5476    -9     
- Misses       3914   3923    +9     
- Partials      368    371    +3     
Flag Coverage Δ
ipv6 23% <40%> (+<1%) ⬆️
py310asyncio 52% <80%> (-1%) ⬇️
py310epolls 53% <80%> (-1%) ⬇️
py310poll 53% <80%> (-1%) ⬇️
py310selects 53% <80%> (-1%) ⬇️
py311asyncio 52% <80%> (-1%) ⬇️
py311epolls 53% <80%> (-1%) ⬇️
py312asyncio 50% <80%> (-1%) ⬇️
py312epolls 51% <80%> (-1%) ⬇️
py37asyncio 50% <80%> (-1%) ⬇️
py37epolls 51% <80%> (-1%) ⬇️
py38asyncio 51% <80%> (-1%) ⬇️
py38epolls 53% <80%> (-1%) ⬇️
py38openssl 51% <80%> (-1%) ⬇️
py38poll 53% <80%> (-1%) ⬇️
py38selects 53% <80%> (-1%) ⬇️
py39asyncio 51% <80%> (-1%) ⬇️
py39dnspython1 51% <80%> (-1%) ⬇️
py39epolls 53% <80%> (-1%) ⬇️
py39poll 53% <80%> (-1%) ⬇️
py39selects 52% <80%> (-1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants