Skip to content

bpo-22807: Expose platform UUID generation safety information. #138

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

Merged
merged 12 commits into from
Feb 18, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion Doc/library/uuid.rst
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,30 @@ If all you want is a unique ID, you should probably call :func:`uuid1` or
a UUID containing the computer's network address. :func:`uuid4` creates a
random UUID.

Depending on support from the underlying platform, :func:`uuid1` may or may
not return a "safe" UUID. A safe UUID is one which is generated using
synchronization methods that ensure no two processes can obtain the same
UUID. All instances of :class:`UUID` have an :attr:`is_safe` attribute
which relays any information about the UUID's safety, using this enumeration:

.. class:: UUID(hex=None, bytes=None, bytes_le=None, fields=None, int=None, version=None)
.. class:: SafeUUID

.. attribute:: SafeUUID.safe
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the current style in uuid.rst, but it might be better use the following style in new documentation:

.. class:: SafeUUID

   .. versionadded:: 3.7

   .. attribute:: safe
      The UUID was generated by the platform in a multiprocessing-safe way.

   ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that looks nice, thanks. The one problem is that SafeUUID still gets labeled as a class in the HTML. Okay, technically it is a class, but it would be nice to have an enum:: markup. I grep'd around in the source and couldn't find any such examples, so I don't know if we support that yet.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. We don't have an enum directive now, but it should be easy to add one. You can add me to nosy list if you create an issue on bugs.p.o :)


The UUID was generated by the platform in a multiprocessing-safe way.

.. attribute:: SafeUUID.unsafe

The UUID was not generated in a multiprocessing-safe way.

.. attribute:: SafeUUID.unknown

The platform does not provide information on whether the UUID was generated
safely or not.

.. versionadded:: 3.7

.. class:: UUID(hex=None, bytes=None, bytes_le=None, fields=None, int=None, version=None, *, is_safe=SafeUUID.unknown)
Copy link
Member

Choose a reason for hiding this comment

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

Currently, the body of UUID() documentation briefly describes all its arguments so I think we can just copy is_safe documentation in Lib/uuid.py

Copy link
Member Author

Choose a reason for hiding this comment

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

+1


Create a UUID from either a string of 32 hexadecimal digits, a string of 16
bytes as the *bytes* argument, a string of 16 bytes in little-endian order as
Expand Down Expand Up @@ -120,6 +142,13 @@ random UUID.
The UUID version number (1 through 5, meaningful only when the variant is
:const:`RFC_4122`).

.. attribute:: UUID.is_safe

An enumeration of :class:`SafeUUID` which indicates whether the platform
generated the UUID in a multiprocessing-safe way.

.. versionadded:: 3.7

The :mod:`uuid` module defines the following functions:


Expand Down
40 changes: 40 additions & 0 deletions Lib/test/test_uuid.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,46 @@ def test_uuid1(self):
equal(((u.clock_seq_hi_variant & 0x3f) << 8) |
u.clock_seq_low, 0x3fff)

@unittest.skipUnless(importable('ctypes'), 'requires ctypes')
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I'd flip the order of the decorators.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1

@unittest.skipUnless(uuid._uuid_generate_time.restype is not None,
'requires uuid_generate_time_safe(3)')
def test_uuid1_safe(self):
u = uuid.uuid1()
# uuid_generate_time_safe() may return 0 or -1 but what it returns is
# dependent on the underlying platform support. At least it cannot be
# unknown (unless I suppose the platform is buggy).
self.assertNotEqual(u.is_safe, uuid.SafeUUID.unknown)

@unittest.skipUnless(importable('ctypes'), 'requires ctypes')
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion: Using self.subTest() would make the following four tests much shorter and IMO easier to understand.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've played a bit with subTest() and I'm not entirely thrilled with it. AFAICT, subtests only print output when they fail, not when they succeed. In verbose mode, I'd want to see all 4 tests printed individually. (That's one of the things I'd like to eventually fix.) For now, I think I'll leave them as separate tests.

def test_uuid1_unknown(self):
# Even if the platform has uuid_generate_time_safe(), let's mock it to
# be uuid_generate_time() and ensure the safety is unknown.
with unittest.mock.patch.object(uuid._uuid_generate_time,
'restype', None):
u = uuid.uuid1()
self.assertEqual(u.is_safe, uuid.SafeUUID.unknown)

@unittest.skipUnless(importable('ctypes'), 'requires ctypes')
def test_uuid1_is_safe(self):
with unittest.mock.patch.object(uuid._uuid_generate_time,
'restype', lambda x: 0):
u = uuid.uuid1()
self.assertEqual(u.is_safe, uuid.SafeUUID.safe)

@unittest.skipUnless(importable('ctypes'), 'requires ctypes')
def test_uuid1_is_unsafe(self):
with unittest.mock.patch.object(uuid._uuid_generate_time,
'restype', lambda x: -1):
u = uuid.uuid1()
self.assertEqual(u.is_safe, uuid.SafeUUID.unsafe)

@unittest.skipUnless(importable('ctypes'), 'requires ctypes')
def test_uuid1_bogus_return_value(self):
with unittest.mock.patch.object(uuid._uuid_generate_time,
'restype', lambda x: 3):
u = uuid.uuid1()
self.assertEqual(u.is_safe, uuid.SafeUUID.unknown)

def test_uuid3(self):
equal = self.assertEqual

Expand Down
39 changes: 33 additions & 6 deletions Lib/uuid.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@

import os

from enum import Enum


__author__ = 'Ka-Ping Yee <ping@zesty.ca>'

RESERVED_NCS, RFC_4122, RESERVED_MICROSOFT, RESERVED_FUTURE = [
Expand All @@ -55,7 +58,14 @@
int_ = int # The built-in int type
bytes_ = bytes # The built-in bytes type

class UUID(object):

class SafeUUID(Enum):
safe = 0
unsafe = -1
unknown = None


class UUID:
"""Instances of the UUID class represent UUIDs as specified in RFC 4122.
UUID objects are immutable, hashable, and usable as dictionary keys.
Converting a UUID to a string with str() yields something in the form
Expand Down Expand Up @@ -104,7 +114,8 @@ class UUID(object):
"""

def __init__(self, hex=None, bytes=None, bytes_le=None, fields=None,
int=None, version=None):
int=None, version=None,
*, is_safe=SafeUUID.unknown):
r"""Create a UUID from either a string of 32 hexadecimal digits,
a string of 16 bytes as the 'bytes' argument, a string of 16 bytes
in little-endian order as the 'bytes_le' argument, a tuple of six
Expand All @@ -128,6 +139,10 @@ def __init__(self, hex=None, bytes=None, bytes_le=None, fields=None,
be given. The 'version' argument is optional; if given, the resulting
UUID will have its variant and version set according to RFC 4122,
overriding the given 'hex', 'bytes', 'bytes_le', 'fields', or 'int'.

is_safe is an enum exposed as an attribute on the instance. It
indicates whether the UUID has been generated in a way that is safe
for multiprocessing applications, via uuid_generate_time_safe(3).
"""

if [hex, bytes, bytes_le, fields, int].count(None) != 4:
Expand Down Expand Up @@ -182,6 +197,7 @@ def __init__(self, hex=None, bytes=None, bytes_le=None, fields=None,
int &= ~(0xf000 << 64)
int |= version << 76
self.__dict__['int'] = int
self.__dict__['is_safe'] = is_safe
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to assert the type of is_safe ? There seem to extensive checking of the rest of the parameters.
Not that I imagine people creating UUID object in their own code, but is_safe feel also a lot like a Boolean.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure it's worth type checking is_safe. The other ones are checked to ensure sane, compliant UUIDs, but is_safe is just a flag with some extra information (which may or may not be helpful to an application). If someone wants to manually instantiate UUIDs with bogus is_safe flags, oh well.


def __eq__(self, other):
if isinstance(other, UUID):
Expand Down Expand Up @@ -472,10 +488,17 @@ def _netbios_getnode():
for libname in _libnames:
try:
lib = ctypes.CDLL(ctypes.util.find_library(libname))
except Exception:
except Exception: # pragma: nocover
continue
if hasattr(lib, 'uuid_generate_time'):
# Try to find the safe variety first.
if hasattr(lib, 'uuid_generate_time_safe'):
_uuid_generate_time = lib.uuid_generate_time_safe
# int uuid_generate_time_safe(uuid_t out);
break
elif hasattr(lib, 'uuid_generate_time'): # pragma: nocover
_uuid_generate_time = lib.uuid_generate_time
# void uuid_generate_time(uuid_t out);
_uuid_generate_time.restype = None
break
del _libnames

Expand Down Expand Up @@ -566,8 +589,12 @@ def uuid1(node=None, clock_seq=None):
# use UuidCreate here because its UUIDs don't conform to RFC 4122).
if _uuid_generate_time and node is clock_seq is None:
_buffer = ctypes.create_string_buffer(16)
_uuid_generate_time(_buffer)
return UUID(bytes=bytes_(_buffer.raw))
safely_generated = _uuid_generate_time(_buffer)
try:
is_safe = SafeUUID(safely_generated)
except ValueError:
is_safe = SafeUUID.unknown
return UUID(bytes=bytes_(_buffer.raw), is_safe=is_safe)

global _last_timestamp
import time
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,10 @@ Extension Modules
Library
-------

- bpo-22807: Add uuid.SafeUUID and uuid.UUID.is_safe to relay information from
the platform about whether generated UUIDs are generated with a
multiprocessing safe method.

- bpo-29576: Improve some deprecations in importlib. Some deprecated methods
now emit DeprecationWarnings and have better descriptive messages.

Expand Down