arvidn / libtorrent

an efficient feature complete C++ bittorrent implementation
http://libtorrent.org
Other
5.23k stars 992 forks source link

TypeError when converting python 32-bit long ('L') to bool in make_settings_pack #1382

Closed cas-- closed 4 years ago

cas-- commented 7 years ago

I found that I could not directly use the save_state settings as settings_pack because the bdecode of the state is representing int as longs and then starting the session with those values fails with:

TypeError: No registered converter was able to produce a C++ rvalue of type bool from this Python object of type long

That is cannot convert 0L would be a concern except that long has been dropped in Python and will result in a SyntaxError on Python3.

Example code:

with open('statefile', 'rb') as _file:
    state = lt.bdecode(_file.read())
settings_pack = state.pop('settings', {})
session = lt.session(settings_pack, flags=0)

An example settings dict output from state:

{'connection_speed': 20L, 'enable_dht': 0L, 'active_seeds': 3L,
 'unchoke_slots_limit': 10L, 'allowed_enc_level': 2L, 'in_enc_policy': 0L, 
'alert_queue_size': 10000L, 'listen_interfaces': '0.0.0.0:63255', 'active_limit': 5L, 
'alert_mask': 861L, 'cache_expiry': 60L, 'out_enc_policy': 0L, 
'upload_rate_limit': 15360L, 'proxy_port': 8080L,
 'connections_limit': 34L, 'active_downloads': 2L, 
'dont_count_slow_torrents': 0L, 'peer_fingerprint': '_DE2000_', 
'download_rate_limit': 10240L, 'prefer_rc4': 1L, 'half_open_limit': 20L, 
'user_agent': 'Deluge/2.0.0.dev881 libtorrent/1.1.1.0', 
'seed_time_limit': 10800L, 'cache_size': 4L, 
'dht_bootstrap_nodes': 'router.bittorrent.com:6881,router.utorrent.com:6881,router.bitcomet.com:6881'}
arvidn commented 7 years ago

do you have your own bencoder in python? I imagine you get the state file in the form of a python dictionary and write it to disk, no? I feel pretty confident that the libtorrent encoder would not include the 'L', and I don't believe that's valid

cas-- commented 7 years ago

The state is lt bencoded and saved directly from save_state:

with open('statefile', 'wb') as _file:
    _file.write(lt.bencode(session.save_state()))
cas-- commented 7 years ago

I wonder if it's a boost version issue as on dev vm with boost 1.55 it's include Ls but on my desktop with 1.58 it is not...

>>> sess.save_state()
{'dht': {'aggressive_lookups': 1L,

>>> sess.save_state()
{'dht': {'aggressive_lookups': 1,

Just to add that the bencoded entries look identical:

>>> lt.bencode(sess.save_state())
'd3:dhtd18:aggressive_lookupsi1e15:block_ratelimiti5e13:block_timeouti300e15:

Oh and using the deluge python bdecode does not add the L.

arvidn commented 7 years ago

oh, I see. This is about the conversion from the C++ representation of the tree structure to python. numbers are represented as signed 64 bit integers on the C++ side. Still though, I would imagine this wouldn't be an interoperability issue as long as a python2 and python3 program communicates via the actual bencoded representation on disk, no?

The "L" just denotes that the python type that's used is a Long and not a Number, is that right?

cas-- commented 7 years ago

Yes the L denotes long vs int in python but it is unneeded as python2 transparently handles this and python3 will raise a SyntaxError encountering it, although would need to verify what happens on Python3. Edit: Python3 is fine because int is old python2 long so doesn't apply here when converting.

The mentioning of the 64-bit reminds me that my dekstop is 64-bit but the dev VM is 32-bit so that must be why the L is appended to ints.

The problem is that you cannot take the save_state and pass it straight into the session constructor as it faults on the conversion from long.

arvidn commented 7 years ago

I see

cas-- commented 7 years ago

Is it possible for this to be fixed in 1.1.2?

I think we ignore the fact that bdecode return 32-bit longs and instead switch attention to make_settings_pack where the actual error occurs. Perhaps this workaround in make_settings_pack would be sufficient:

- p.set_bool(sett, extract<bool>(value));
+ p.set_bool(sett, extract<bool>(value != 0));

My only concern would be where else could this be a potential issue in the bindings so would having a long_to_bool converter be a better solution? I don't quite understand why boost python can't handle this though.

A simpe test would be one or both of these:

session = lt.session({'enable_dht': 1L})
session.apply_settings({'enable_dht': 1L})
cas-- commented 7 years ago

The bdecode outputing long instead of int is still a minor issue on 32-bit python2 platforms:

>>> lt.bencode({'test':0})
'd4:testi0ee'
>>> lt.bdecode('d4:testi0ee')
{'test': 0L}
>>> bencode.bdecode('d4:testi0ee') # python bencode module
{'test': 0}

However looking in bdecode.hpp I think it's because boost::int64_t is being used for int values.

arvidn commented 7 years ago

actually, it sounds like when the settings are converted to python, bool settings should be turned into proper python bools. the problem now is that the bindings are trying to extract a bool from a long. that shouldn't be a long to begin with, should it?

arvidn commented 7 years ago

adding a converter from long -> bool seems reasonable too. just for python2 I take it, right?

cas-- commented 7 years ago

For bencoding there is no bool just int which is where the long issue comes from. The output from get_settings does return python bool.

adding a converter from long -> bool seems reasonable too. just for python2 I take it, right?

That would be fine. Yes just python2

arvidn commented 7 years ago

I guess one reason why this is a problem is that the API for apply_settings() is only "accidentally" compatible with being bencoded and restored. And in fact, in this case, it's not quite compatible. I imagine those bools get converted to ints when you bencode it and then back to longs when you decode it.

If you would use save_state() and load_state(), this would not be a problem, because those functions specifically work with bencoded structures.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.