Dynamically adding mappings #107

Activity

1. attwad commented on Oct 7, 2019

   

   attwad

   on Oct 7, 2019

   Owner

   More actions

   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.
2. jobor019 commented on Oct 8, 2019

   

   jobor019

   on Oct 8, 2019

   Author

   More actions

   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?
3. PaulWGraham commented on Oct 9, 2019

   

   PaulWGraham

   on Oct 9, 2019

   Last edited by PaulWGraham

   More actions

   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.
4. attwad commented on Oct 10, 2019

   

   attwad

   on Oct 10, 2019

   Owner

   More actions

   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.
5. jobor019 commented on Oct 10, 2019

   

   jobor019

   on Oct 10, 2019

   Author

   More actions

   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.
6. PaulWGraham commented on Oct 10, 2019

   

   PaulWGraham

   on Oct 10, 2019

   More actions

   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.
7. attwad commented on Oct 11, 2019

   

   attwad

   on Oct 11, 2019

   Owner

   More actions

   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.
8. PaulWGraham commented on Oct 11, 2019

   

   PaulWGraham

   on Oct 11, 2019

   More actions

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