niccokunzmann / python_dhcp_server

a dhcp server in python
MIT License
89 stars 32 forks source link

Threading/Queue Issue #12

Closed waukeerules closed 2 years ago

waukeerules commented 5 years ago

I have been running into the following error messages while running the DHCP server. I don't understand multithreating enough to know how to resolve the issue. Has anyone seen this issue before? It seems to happen the longer I leave the server running or how many devices I boot up at once.

Traceback (most recent call last): File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 529, in run self.update(1) File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 448, in update self.received(packet) File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 455, in received if not self.transactions[packet.transaction_id].receive(packet): File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 167, in receive self.received_dhcp_request, (packet,), ) File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 139, in do_after self.queue.put((time.time() + seconds, func, args, kw)) known ip: 192.168.0.79 File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 143, in put self._put(item) File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 227, in _put heappush(self.queue, item) TypeError: '<' not supported between instances of 'method' and 'method'

Traceback (most recent call last): File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\threading.py", line 916, in _bootstrap_inner self.run() File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\threading.py", line 864, in run self._target(*self._args, **self._kwargs) File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 131, in _delay_response_thread p = self.queue.get(timeout=1) File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 174, in get item = self._get() File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 230, in _get return heappop(self.queue) TypeError: '<' not supported between instances of 'method' and 'method'

Exception in thread Thread-1: Traceback (most recent call last): File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\threading.py", line 916, in _bootstrap_inner self.run() File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\threading.py", line 864, in run self._target(*self._args, **self._kwargs) File "C:\Users\user\Desktop\Python\SSH Tool\IOS PnP\dhcp.py", line 131, in _delay_response_thread p = self.queue.get(timeout=2) File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 174, in get item = self._get() File "C:\Users\user\AppData\Local\Programs\Python\Python36\lib\queue.py", line 230, in _get return heappop(self.queue) TypeError: '<' not supported between instances of 'method' and 'method'

waukeerules commented 5 years ago

I found that the errors only occur when multiple DHCP requests are initiated at the same exact time.

greggzj commented 5 years ago

hello @waukeerules , seems very interesting on your issue... Frankly speaking, I am also fresh on this... unitl now what I only know is my following comments , @niccokunzmann please do not hesitate to point out if I make any mistakes... thanks in advance.

Take the first part of your log as an example, when calling self.queue.put((time.time() + seconds, func, args, kw)), priorityqueue use heapq and inside heapq it uses operator < to judge for the priority of current input tuple and previous input tuple.

When comparing the tuple, it first judges the first item of tuple : time.time() , it finds that current tuple and previous tuple has the same value for time.time() (really amazing here ...) , then it compare the second field of tuple , but this time func is a method and can not be compared , thus cause the TypeError: '<' not supported between instances of 'method' and 'method'. (Refer to python code of heapq.py (function _siftdown(heap, startpos, pos):)and here for more information.

So , for conclusion, Seems that the reason why heapq complains: `TypeError: ' is that there is one tuple in previous priority queue with the same timestamp value of the current input one stored in priorityqueue! sounds really amazing...

Unfortunately, I do not know much about multithread either, but it seems that using current python standard library PriorityQueue and storing timestamp value in queue ( actually in python list) used as priority identifier may be not a good idea for they may be the same value .....

May be we can find another value used as identifier instead of time.time() stored in the priorityqueue. Or using a self implement priorityqueue to avoid such a problem^_^

what do u think @niccokunzmann

greggzj commented 5 years ago

Hello @waukeerules ,

I modify the dhcp.py , using a simple self-implement PriorityQueue class instead of the official one to avoid the timestamp same value problem. I test it on my bench it works. But for multiple device condition I need your help.

Could you please help me to have a try ? just unzip the file and replace the old one . Hoping for your feedback ! thanks in advance

dhcp.zip

niccokunzmann commented 5 years ago

Thanks for reporting this issue! I think, I assumed this case never to turn up.

Yes to this: https://github.com/niccokunzmann/python_dhcp_server/issues/12#issuecomment-509946979

To me, it seems this is the culprit: https://github.com/niccokunzmann/python_dhcp_server/blob/32ba283a4ca61ff6be322f546ec796f0ecc59c77/server/dhcp.py#L139 In this line 139, I assume that the times are different or at least that the functions can be ordered (which is possible in Python 2 bu not on Python 3).

If we put an additional item in the list, we would also need to change this line: https://github.com/niccokunzmann/python_dhcp_server/blob/32ba283a4ca61ff6be322f546ec796f0ecc59c77/server/dhcp.py#L128

My solution would be to add a number there which is unique and increased inside a lock, so that there is no multi-threading issue.

_id_lock = threading.Lock()
_id = 0
def get_id():
    global _id
    with _id_lock:
        _id += 1
        return _id

As as an answer to https://github.com/niccokunzmann/python_dhcp_server/issues/12#issuecomment-509975402, @greggzj if you create a pull request, we can talk about this and even download the source code of it as a zip file and see the change. I do not know what you did and what the change is and this makes it harder for me, in case this is the accepted solution, to add the change into the source code here.

@greggzj @waukeerules, how would you like to go on from here?

greggzj commented 5 years ago

@niccokunzmann thanks for feedback and sorry for the inconvenience.

I create a pull request #13 and the newly implemented PriorityQueue class 1) no need to modify this line for the self implement priorityqueue class always return item itself exclude the priority identifier index: https://github.com/niccokunzmann/python_dhcp_server/blob/32ba283a4ca61ff6be322f546ec796f0ecc59c77/server/dhcp.py#L128 2) Timestamp value is still used as before.

Seems your solution also works. Which one do u prefer ?

waukeerules commented 5 years ago

Thanks for looking into this issue. I was able to create a simple workaround by changing the queuing method to -first in, first out:

self.queue = queue.Queue()

niccokunzmann commented 2 years ago

@waukeerules, thanks.... Yep, that is the simplest solution. I used this - the thing is that the packets are re-ordered automatically if it is a queue. So, that is fine and it does not need to be a priority queue. I am closing this. If anyone finds out that this re-appears, please let me know.

d0d50df