Be careful with .pop
Sergey Kislyakov

Sergey Kislyakov @defman

Location:
Russia
Joined:
Jan 2, 2017

Be careful with .pop

Publish Date: Mar 26 '19
9 5

I've been fighting with a weird bug in my app: if I'm sending a push notification to 2+ users, only the first one receives the notification.

The bug was that I .poped the message key from the data.

Here's how it works (pseudo-code):

def send(users: [User], data: dict):
    notifications = reduce(lambda acc, user: acc + [Notification(user, data)], users, [])
    db.bulk_insert(notifications)
    for notification in notifications:
        notification.send()

...

def Notification.send(self):
    message = self.data.pop('message') 
    Firebase.send(message, self.data)
Enter fullscreen mode Exit fullscreen mode

The first notification will be sent and I thought that message would be popped out of self.data, but actually it would be popped out of the dict that self.data points to! That means that every Notification object that has been initialized in the send function using Notification(user, data) (and not Notification(user, data.copy())) would not have data['message'] anymore, leading to unexpected behavior.

If you really need to get rid of a key in a dict, you should .copy() it before mutating.
If you don't - just stick to .get().

Comments 5 total

  • rhymes
    rhymesMar 26, 2019

    Good catch Defman!

    Keep in mind that copy() does not create a deep copy so if you modify the Notification objects... both references will point to the same Notification object that you modified :D

    If you have memory constraints and you want to let the GC do its work you could investigate the usage of weakref:

    A primary use for weak references is to implement caches or mappings holding large objects, where it’s desired that a large object not be kept alive solely because it appears in a cache or mapping.

    • Sergey Kislyakov
      Sergey KislyakovMar 26, 2019

      Keep in mind that copy() does not create a deep copy so if you modify the Notification objects... both references will point to the same Notification object that you modified :D

      I don't quite understand that. Doesn't {"a": 1, "b": {"c": 2}}.copy() create a copy of a dict (and allocate it in the memory) that would pass assert id(Notification1.data) != id(Notification2.data) (so by modifying Notification1.data I would not affect Notification2.data)?

      • rhymes
        rhymesMar 26, 2019

        copy() performs a shallow copy:

        In [1]: inner_dict = {"c": 2}

In [2]: d = {"a": 1, "b": inner_dict}

In [3]: c_d = d.copy()

In [4]: inner_dict["x"] = 3

In [5]: inner_dict
Out[5]: {'c': 2, 'x': 3}

In [6]: d
Out[6]: {'a': 1, 'b': {'c': 2, 'x': 3}}

In [7]: c_d
Out[7]: {'a': 1, 'b': {'c': 2, 'x': 3}}

In [8]: id(d['b']), id(c_d['b'])
Out[8]: (4364454576, 4364454576)

        as you can see both d and its copy c_d point to the same inner_dict which when modified it reflects on both the original and the copy.

        TLDR; if any of the values of the dict that's about to be copied is a reference to an object (in this case another dictionary, in your case a Notification object) then the original and the copy will point to the same object.

        • Sergey Kislyakov
          Sergey KislyakovMar 26, 2019

          I see. Thanks!

          In my case, data does not hold a Notification object. It doesn't hold any references at all, actually. It's a simple dict that looks like this: {"message": "my message", "some_attr": 123}.

  • Mikael Klages
    Mikael KlagesMar 28, 2019

    If you're working with reference-types, it's probably better to never use mutating functions unless you absolutely have to.

Add comment