Dynamically adding mappings

[jobor019]

Hello,

I'm porting a project from python2 to python3 where there's a need to dynamically add new handlers to an OSC server while it's still running. Currently, this causes a runtime error, see example below:

from pythonosc import dispatcher, osc_server


class DynamicExample:

    def __init__(self):
        disp = dispatcher.Dispatcher()
        disp.map("/new_callback", self.new_callback)
        self.server = osc_server.BlockingOSCUDPServer(("127.0.0.1", 8081), disp)
        self.server.serve_forever()

    def new_callback(self, _address, *_args):
        self.server.dispatcher.map("/other_address", self.dummy_function)

    def dummy_function(self, address, *args):
        pass


if __name__ == '__main__':
    DynamicExample()

Sending a message to /new_callback causes the following error:

----------------------------------------
Exception happened during processing of request from ('127.0.0.1', 58368)
Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/socketserver.py", line 316, in _handle_request_noblock
    self.process_request(request, client_address)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/socketserver.py", line 347, in process_request
    self.finish_request(request, client_address)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/socketserver.py", line 360, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/socketserver.py", line 720, in __init__
    self.handle()
  File "/usr/local/lib/python3.7/site-packages/pythonosc/osc_server.py", line 29, in handle
    self.server.dispatcher.call_handlers_for_packet(self.request[0], self.client_address)
  File "/usr/local/lib/python3.7/site-packages/pythonosc/dispatcher.py", line 193, in call_handlers_for_packet
    for handler in handlers:
  File "/usr/local/lib/python3.7/site-packages/pythonosc/dispatcher.py", line 161, in handlers_for_address
    for addr, handlers in self._map.items():
RuntimeError: dictionary changed size during iteration
---------------------------------------

Would it be possible to solve this somehow?

[attwad]

Interesting use case, we'd need to make dispatcher thread-safe by using a threading.Lock around dispatcher._map.
I don't expect the performance hit to be an issue (taking a lock takes a few dozen nanoseconds and hopefully your application doesn't change the mappings too often), we can make this the default.
Contributions are welcome.

[jobor019]

I could have a look at it, but are you sure this is a threading issue? As far as I can tell, both the iteration over the dict (dispatcher.py:193) and the callback function that modifies the dict (new_callback, my implementation above) are executed on the same thread, but perhaps I'm misunderstanding something?

[PaulWGraham]

I couldn't find anything dispositive in the python documentation but from what I can tell from looking at it with Process Explorer on Win10 it doesn't seem that vanilla generators spawn another thread.

I think the reason this line,
for addr, handlers in self._map.items():
causes an error is because self._map.items() returns an instance of the class dict_items. The problem with dict_items that when the size of the original dict changes so does the length of the dict_items. For example:

import collections
dd = collections.defaultdict(list)
items = dd.items()
copy_of_items = [*items]
print(len(items)) # prints 0
print(len(copy_of_items)) # prints 0
dd["foo"] = "bar"
print(len(items)) # prints 1
print(len(copy_of_items)) # prints 0

I haven't tested it but what I think is happening is that is that when the iteration resumes from,
yield from handlers
it discovers that length of the dict_items has changed because self._map has changed thus causing the error.

It is essentially the same error as caused by the following code albeit in a more roundabout way:

import collections

dd = collections.defaultdict(list)
dd["foo"] = "bar"
items = dd.items()

for _ in items:
  dd["foos"] = "ball"

I only found this project today and I really haven't dug too deep but from a cursory examination it looks like it might be possible to change handlers_for_address so that instead of returning a generator it just returns a tuple. This would make it simpler to set up guards for thread safety for the requested functionality due to not having to deal with the saved state used by the generators.

[attwad]

thanks for the investigation, if you can confirm with a local change that tuples do not produce that error then we could change to that, there's little reason to use generators over tuples currently.

[jobor019]

I can indeed confirm that replacing the line

python-osc/pythonosc/dispatcher.py

Line 161 in 7f00183

for addr, handlers in self._map.items():

with for addr, handlers in tuple(self._map.items()): makes the example code above run without errors and behave as intended.

[PaulWGraham]

Sorry. I think the last paragraph of my last comment has lead to some confusion. Changing the line
for addr, handlers in self._map.items():
may suppress the original error but it definitely doesn't make adding a map dynamically a thread safe thing to do. When I said:

I only found this project today and I really haven't dug too deep but from a cursory examination it looks like it might be possible to change handlers_for_address so that instead of returning a generator it just returns a tuple. This would make it simpler to set up guards for thread safety for the requested functionality due to not having to deal with the saved state used by the generators.

I was floating the notion that it might make sense to:

1. Remove all the yields from handlers_for_address to make things easier.

2. Instead of returning generators from handlers_for_address return an iterable like a tuple or a list.

3. Add locks to the critical sections.

But I really don't know the code base well enough to know if removing the generator based code is feasible or to make suggestions concerning thread safety.

[attwad]

Alright, if getting rid of generators makes it work in a single thread let's do that, we can add locks later if people want to make the dispatcher threadsafe (we can create another issue for that).

FYI I'm going away for 2 weeks, I will have little to no connectivity so please bare with the slow review if you send a PR, thanks.

[PaulWGraham]

For single threaded operation I'm pretty sure all that needs to change is line 161 as @jobor019 confirmed above.