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

Fix winapi initialization on Python 2.5 on Windows #253

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@akx
Contributor

akx commented Jan 20, 2014

This regression (apparently introduced in 1389278) was uncovered during testing #193/#230.

Rudimentary test result below. Doesn't seem to break 2.6/2.7 (though more eyes looking at get_thread_ident() might not hurt).

PYTHON: C:\Python25 -- LIBRARY: tip.paramiko -- TEST: agent_init.py
  Traceback: AttributeError: 'module' object has no attribute 'USHORT'
PYTHON: C:\Python26 -- LIBRARY: tip.paramiko -- TEST: agent_init.py
PYTHON: C:\Python27 -- LIBRARY: tip.paramiko -- TEST: agent_init.py
PYTHON: C:\Python25 -- LIBRARY: paramiko-git-akx -- TEST: agent_init.py
PYTHON: C:\Python26 -- LIBRARY: paramiko-git-akx -- TEST: agent_init.py
PYTHON: C:\Python27 -- LIBRARY: paramiko-git-akx -- TEST: agent_init.py
except AttributeError:
return id(current_thread()) # Not safe but best we can do
try:

This comment has been minimized.

@lndbrg

lndbrg Jan 20, 2014

Contributor

Same try except as on line 33?

except ImportError:
from threading import currentThread as current_thread
def get_thread_ident():

This comment has been minimized.

@lndbrg

lndbrg Jan 20, 2014

Contributor

Can't http://docs.python.org/2/library/thread.html#thread.get_ident be used here instead? It existed in 2.5 and in all other paramikos supported versions.

@akx

This comment has been minimized.

Contributor

akx commented Jan 20, 2014

Thanks @lndbrg (and sorry for the history clobbering -- wanted the earlier mess out of sight).

@lndbrg

This comment has been minimized.

Contributor

lndbrg commented Jan 20, 2014

Don't think this will work for all py versions.

py3 renamed thread to _thread.
The recommended interface for py3 is threading (py34 introduces threading.get_ident).
I think the compat matrix is:

interface: py2.5 py2.6 py2.7 py3.2 py3.3 py3.4
thread x x x - - -
threading x x x x x x
_thread - - - x x x
interface: py2.5 py2.6 py2.7 py3.2 py3.3 py3.4
thread.currentThread x x x - - -
thread.current_thread - x x - - -
threading.currentThread x x x - - -
threading.current_thread - x x x x x
threading.get_ident - - - - - x
thread.get_ident x x x - - -
_thread.get_ident - - - x x x

Looks like you need to import threading and check for currentThread or current_thread and then call get_ident on that.

@lndbrg

This comment has been minimized.

Contributor

lndbrg commented Jan 20, 2014

Updated the above comment. Threading exists on all of the python platforms we try to support.

@akx

This comment has been minimized.

Contributor

akx commented Jan 20, 2014

Now that's thorough. Thank you again.

So, I suppose

def get_thread_ident():
  try:
     return thread.get_ident()
  except AttributeError:
     return threading.current_thread().ident

would do the trick?

@lndbrg

This comment has been minimized.

Contributor

lndbrg commented Jan 20, 2014

With the updated matrix I think this would do the trick:

def get_thread_ident():
  current_thread =  None
  try:
     current_thread = threading.currentThread()
  except AttributeError:
      current_thread = threading.current_thread()
  return current_thread.get_ident()
@akx

This comment has been minimized.

Contributor

akx commented Jan 20, 2014

The original bug was that _MainThread on Python 2.5 does not have an ident member, so I don't think that that'd work.

@lndbrg

This comment has been minimized.

Contributor

lndbrg commented Jan 20, 2014

That's because ident is new in 2.6 on the threading module. But get_ident exists on thread and _thread.

something like:

try:
   import _thread as thread
except ImportError:
   import thread

thread.get_ident()

Untested.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 20, 2014

If this is becoming too much of a pain, feel free to punt on it - I mentioned 2.5 in #193 as a matter of course, but once the Python 3 support lands we will discontinue 2.5 support anyway. So this might be moot.

Dig if you want - it'd be nice for the last pre-Py3 releases to work well on 2.5 for Windows - but I'm OK to merge #193 without this functioning if it'll save time.

EDIT: note that I've only skimmed this so far, recent comments sound like you're still kicking things around? If that's not the case and this can be merge as-is, that's cool too.

@lndbrg

This comment has been minimized.

Contributor

lndbrg commented Jan 21, 2014

Ignoring py2.5, this looks like the best solution: threading.current_thread().ident
@akx what do you say?

@akx

This comment has been minimized.

Contributor

akx commented Jan 21, 2014

Well, I flipped the order the identity sources are tried, so now Py2.5 compatibility is the uncommon case.

I think this (and #230) would leave the last pre-Py3 version compatible with Win+Py2.5. At least for the agent/winapi bit, anyway...

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 21, 2014

Thanks again. Spoke to @lndbrg on IRC a bit and committed a7ea048 to clean things up a bit further (re: his above comments.)

Merged into mainline (1.10 -> master), will close this and the other PR (had to cherry pick so it won't autoclose) and move discussion back into #193.

@bitprophet bitprophet closed this Jan 21, 2014

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