Dynamically adding mappings

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?

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.

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?

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.

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.

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.

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:

Remove all the yields from handlers_for_address to make things easier.
Instead of returning generators from handlers_for_address return an iterable like a tuple or a list.
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.

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.

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