Fix for AsyncUDP's packet handling callback #3290
Conversation
…ack for both pass-as-reference and pass-as-copy versions. See espressif#3287 and espressif#3288.
|
The copy constructor needs to call a function, hence its long-form version. :/ |
| @@ -682,9 +698,8 @@ void AsyncUDP::_recv(udp_pcb *upcb, pbuf *pb, const ip_addr_t *addr, uint16_t po | |||
| if(_handler) { | |||
| AsyncUDPPacket packet(this, this_pb, addr, port, netif); | |||
| _handler(packet); | |||
| } else { | |||
| pbuf_free(this_pb); | |||
There was a problem hiding this comment.
on destruct the packet will free the pbuf. if it does not, that should be fixed there, or where the packet is copied
There was a problem hiding this comment.
I'm not quite following what you're saying. pbuf_free, according to the code, needs to be called at least once, even if the handler isn't used. As soon as the handler is used with the new AsyncUDPPacket object, its constructor and destructor will pbuf_ref and pbuf_free, respectively. That's why the copy constructor needs to add a pbuf_ref, to balance things out if the pass-as-copy form of the callback is used.
What are your thoughts on the logic in #3288? I tested the code with my example from #3287 using both pass-as-reference and pass-as-copy forms.
There was a problem hiding this comment.
Another possibility for a fix: Don't call pbuf_ref in the constructor or copy constructor, and don't call pbuf_freein the destructor. Have all management outside the AsyncUDPPacket object. However, code that currently exists and assumes the class will manage this will break.
| memcpy(_remoteMac, packet._remoteMac, sizeof(_remoteMac)); | ||
| pbuf_ref(_pb); | ||
| } | ||
|
|
||
| AsyncUDPPacket::~AsyncUDPPacket() | ||
| { | ||
| pbuf_free(_pb); |
There was a problem hiding this comment.
here the pbuf should be freed. should take care of pbuf_ref as on that depends if pbuf_free will work
There was a problem hiding this comment.
It is freed, as you point out, in the destructor. This balances the call to pbuf_ref in the constructor. That's why my change also adds a call to pbuf_ref in the copy constructor. This solves the imbalance.
There was a problem hiding this comment.
As written, the code won't successfully work for both pass-as-reference and pass-as-copy forms, no matter where the pbuf_free call is used in AsyncUDP::_recv, either outside or inside an else. That's why adding to the copy constructor solves the problem for both cases, if the pbuf_free is moved outside the if/else.
There was a problem hiding this comment.
I have tried your fix before and it did not work in many situations. so I reverted it back. you could look in the history... should be there :) need to figure out better that pbuf ref/unref count and not leak or double free
There was a problem hiding this comment.
Interesting. Could you show me some situations in which my fix fails? I’d like to see if I can resolve them. All I know is that my fix doesn’t fail in any of my tests.
Unless... what are you saying about my fix doesn’t work? From browsing your other code, it looks like you just moved around the pbuf_free In AsybcUDPPacket::_recv without adding code to the copy constructor that calls pbuf_ref. My fix consists of two parts: moving around that pbuf_free call outside the else and modifying the copy constructor.
There was a problem hiding this comment.
unfortunately I can not recall...
There was a problem hiding this comment.
So you’re saying that in the past you’ve already tried adding this fix with changing the copy constructor and it didn’t work? What do you think of the logic in #3288? I don’t see what’s wrong with it.
Could you at least show which commits you undid where the fix didn’t work?
[I’m not saying my solution is not wrong or incorrect, I just don’t see its error. I’d love to help fix this.]
|
@me-no-dev any news on this issue ? :) |
|
@thbl I just use my own fork (I haven't looked at this in over a year, so I'm not sure what the current status is). I'm still not sure what the claimed problem is with my fix, only that it "doesn't work", but I disagree. |
|
@thbl Looks like @me-no-dev took this fix about 2 months ago but didn't use my PR: 8134a42 I still think the copy constructor should use a |
This is a fix for both pass-as-reference and pass-as-copy versions. See #3287 (original bug) and #3288 (reasoning).