ambrop72 / badvpn

NCD scripting language, tun2socks proxifier, P2P VPN
Other
1.86k stars 603 forks source link

confused me:why use an empty job for sync in listener_accept_func #100

Open 578141611 opened 4 years ago

578141611 commented 4 years ago

` err_t listener_accept_func (void arg, struct tcp_pcb newpcb, err_t err) {

...
SYNC_DECL
SYNC_FROMHERE
...

DEAD_ENTER(client->dead_aborted) SYNC_COMMIT DEAD_LEAVE2(client->dead_aborted)

// Return ERR_ABRT if and only if tcp_abort was called from this callback.
return (DEAD_KILLED > 0) ? ERR_ABRT : ERR_OK;

` I can not understand why you need to handle ERR_ABRT? In my opinion,there is only one thread in tun2socks,then we occpied the thread in function "listener_accept_func". can you answer me. thank you. @ambrop72

ambrop72 commented 4 years ago

Hi, lwIP needs us to return ERR_ABRT from the listener callback if we have tcpabort'ed the client. If we are not very careful to do that, lwIP could crash or misbehave. DEAD* is a mechanism to allow us to determine if a certain event has occurred in a certain time frame during processing. In this case, if DEAD_KILL_WITH is called in client_abort_pcb within SYNC_COMMIT (which directly runs pending job handlers), then DEAD_KILLED will be > 0. The whole thing is kind of a hack but is necessary.

ambrop72 commented 4 years ago

On the other hand I suppose the code could be changed to just do the minimum necessary in listener_accept_func, instead of running event loop processing, i.e. no SYNC_COMMIT and related stuff, always return ERR_OK. But it would not be optimal since we would delay sending/receiving that can be done right, so lwIP might do something suboptimal because of that. Probably not a big issue though.

ambrop72 commented 4 years ago

Ah, I think I remember, generally the pattern is that within a lwIP callback I directly run event loop jobs that have resulted from whatever actions were just started (e.g. adding a packet to a buffer), to make sure that all data processing that can be done is done right away - this is what the SYNC_COMMIT does. Without this, if lwIP gives us another packet right afterward, we might not be able to place it in the buffer at all, due to the way that the data processing pipeline works - the processing from the previous packet might be pending on the job queue still, so not ready to handle another packet.

578141611 commented 4 years ago

Thank you. @ambrop72