Random ID should beginning with 1

[SunFulong]

Fix both in id and rid function.

[om26er]

Looks good

[oberstet]

Looks good

what about return struct.unpack("@Q", os.urandom(8))[0] & _WAMP_ID_MASK + 1 ?

I assume & and + are both left-associative in Python ;) not sure, is it?

IOW: what I want to say, the above line can spill out IDs outside the allowed domain

[SunFulong]

the above line can spill out IDs outside the allowed domain

Added parentheses, should works now

[oberstet]

I don't think your last change fixes the problem.

There is a value v for which

struct.unpack("@Q", v)[0] & _WAMP_ID_MASK + 1

will exceed the allowed scope which & _WAMP_ID_MASK is meant to enforce.

[SunFulong]

Only & will limit result in range [0, 2 ** 53 - 1], we expect [1, 2 ** 53]

[oberstet]

>>> import struct
>>> from autobahn.util import _WAMP_ID_MASK
>>> 
>>> val = b"\x00\x00\x00\x00\x00\x00\x00\x00"
>>> struct.unpack("@Q", val)[0] & _WAMP_ID_MASK
0
>>> struct.unpack("@Q", val)[0] & _WAMP_ID_MASK + 1
0
>>> struct.unpack("@Q", val)[0] + 1 & _WAMP_ID_MASK
1
>>> 
>>> val = b"\x00\x1f\xff\xff\xff\xff\xff\xff"
>>> struct.unpack("@Q", val)[0] & _WAMP_ID_MASK
9007199254683392
>>> struct.unpack("@Q", val)[0] & _WAMP_ID_MASK + 1
9007199254740992
>>> struct.unpack("@Q", val)[0] + 1 & _WAMP_ID_MASK
9007199254683393
>>> 
>>> 2**53
9007199254740992
>>> 

---

>>> struct.pack("@Q", 0)
b'\x00\x00\x00\x00\x00\x00\x00\x00'
>>> struct.pack("@Q", 1)
b'\x01\x00\x00\x00\x00\x00\x00\x00'
>>> struct.pack("@Q", int(2**53))
b'\x00\x00\x00\x00\x00\x00 \x00'
>>> struct.pack("@Q", int(2**53 - 1))
b'\xff\xff\xff\xff\xff\xff\x1f\x00'

fwiw, I'd felt much more comfortable with sth along (ideally added as a new CI test):

def rid(value: Optional[int]=None):
    if value is None:
        _value = os.urandom(8)
    else:
        _value = ..
    return (struct.unpack("@Q", value)[0] & _WAMP_ID_MASK)

# test:
rid(0)
rid(1)
rid(2**53 - 1)
rid(2**53)

[oberstet]

oh, shit! how did

@ native

ever get into the code?

independent of the actual issue, ++1 for forcing a machine independent byte order, and +1 for big endian;)

[SunFulong]

Yes, you realize, in fact, things are much more serious!

[oberstet]

luckily, it doesn't seem to be used anyways:

(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ find autobahn/ -name "*.py" -exec grep -Hi " rid(" {} \;
autobahn/util.py:# Performance comparison of IdGenerator.next(), id() and rid().
autobahn/util.py:#   rid()               106.1s
autobahn/util.py:#   rid()               196.4s
autobahn/util.py:def rid():
(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ find ../crossbar/crossbar -name "*.py" -exec grep -Hi " rid(" {} \;
(.venv) oberstet@intel-nuci7:~/scm/crossbario/autobahn-python$ 

the word "Performance" hints at why it might have been thought of desirable (native byte order) in the first place ... bad bad

[SunFulong]

In fact, we can also change MASK to 2 ** 53 - 1, should work both byte-order.

[oberstet]

# 8 byte mask with 53 LSBs set (WAMP requires IDs from [1, 2**53]
_WAMP_ID_MASK = 2 ** 53 - 1

sorry, I won't merge such "hacks"

it obfuscates things further (the -1), and the comment is now wrong (as the value misses values which are indeed allowed, and hence it is no longer a mask)

just make the function check for 0, and if so, self-call/iterate .. straight forward, easy to understand, no magic, no obfuscation, ..

it's just 1-2 lines .. I don't care at all about the "loss of performance" because of an additional branch or what. buy a faster machine is easier;)

[oberstet]

how about

>>> val = 0
>>> while not val:
...     val = secrets.randbits(53)
... 
>>> return val
5543878694689356

[SunFulong]

I don't think it's clear than mine last push

[oberstet]

I don't think it's clear than mine last push

autobahn-python/autobahn/util.py

Line 301 in f084051

return struct.unpack("@Q", os.urandom(8))[0] & _WAMP_ID_MASK or 2 ** 53

of course that's a matter of taste;)

but in any case, I like your change too! it has the advantage of not changing the underlying python functions used so much (as mine)

so let me have a last look before merge ... couple of minutes, I'm busy with sth ..

[oberstet]

alright: #1639

sorry, the discussion takes longer than coding;) but I'd also be cool with you changing your PR and we merge that, or we merge mine, doesn't matter .. but if you update, pls note eg

>>> struct.unpack("@Q", b"\x00\x1f\xff\xff\xff\xff\xff\xff")[0]
18446744073709494016
>>> struct.unpack(">Q", b"\x00\x1f\xff\xff\xff\xff\xff\xff")[0]
9007199254740991

and pls also the other bits (eg changelog) ...

[oberstet]

cool, thank you! looks almost ready. I promise;)

pls also the other bits (eg changelog) ...

https://github.com/crossbario/autobahn-python/pull/1639/files#diff-b7f8a8784d5eff6e6b1b2c2529b831cd86132c49699fe48e0d23c17ad4e2d7deR12

people are actually relying on / asking for proper changelog

the first word should be fix: or new: ... in this case, "fix"

ideally with link to an issue number - in this case, there is none .. only a PR number ... which is "ok for me", but let me emphasize: I did have discussions .. people want and expect proper docs ... follow strict rules like: no change at all without an issue first .. anyways, just saying;)

[SunFulong]

I don't know should I direct update the version, or leave to (unpublished)

Sorry, just now found you've already pushed PR #1639

[oberstet]

awesome! thank you very much! I'll let the CI finish, and then merge!

I don't know should I direct update the version, or leave to (unpublished)

it's ok, just leave it for now.

I do want to push a release soonish .. and the version number actually is already prepared for that new release

the handling of the version numbers, and also when a new release is actually published is not as I ideally would wish for:(

• eg the version should be [r]-dev[n] during the whole development cycle up to the new release [r]
• once development has finished for a cycle (and when that is is also sth unclear), only then should the version be bumped to [r] in one final commit, then tagged, and only then published.
• once that is done, one new commit with [r+1]-dev1 should be pushed immediately

sth like that;)

[oberstet]

Sorry, just now found you've already pushed PR #1639

no worries!

I closed / deleted it: #1639

it was just quicker and more precise to "comment" with a full PR;)

thank you for contributing!

[SunFulong]

once that is done, one new commit with [r+1]-dev1 should be pushed immediately

Right, if the last update change log leave [r+1]-dev1 section, I can put the fix direct in there, so I waste my time to find the rule in repo files and waste your time to write #1637 (comment)

[oberstet]

have dev processes and clear rules, avoiding "wasting your OR my time": ++1

couldn't agree more=)

that should be the goal

and yes, it is neither properly documented (for "unwritten" rules we do have and use) nor complete (for rules we should have, but don't have agreed on) ...

well, that would be another issue;)

"define and document development and release process and rules for library developers"