tcp/pcb: remove urx_close_t, to which nothing binds

[yomimono]

Proposed fix for #153 .

[talex5]

Ha! Well, that solves that problem then :-)

BTW, is the recursive call to rx_application_t correct? I still don't understand what this code is trying to do, although this patch seems correct anyway, since it's just removing dead code.

[ Travis failed with another timeout - restarted. ]

[yomimono]

I've gone back and forth on whether the recursive call is correct depending on which line of code is in front of my face at the time. It's possible that we would receive multiple packets after we've closed the connection, in which case whatever writes to the rx_data mvar might have some distress from trying to write to an mvar which is already full.

Also, this thread is run in a choose block with the transmission and window scaling threads which also don't terminate, so terminating after we get None might have some undesirable knock-on effects from causing this block to return. @balrajsingh ?

[yomimono]

("whatever writes to the rx_data mvar" is https://github.com/mirage/mirage-tcpip/blob/master/tcp/segment.ml#L144-L207 , FWIW)

[talex5]

I'm pretty sure that choose is wrong too. If one of the threads exits, we still need to be able to cancel the others (which is the only thing we use it for I think). Probably it should be a pick (and it would be more efficient to use pick [a;b;c] rather than using the binary operator, which creates two threads).

[balrajsingh]

I think you are right there. I do remember being confused about the
semantics of pick/choose/join... Please do fix.

About the FIN, the packets are delivered in order to the application so
nothing can follow an end of steam, so removing the recursive call on the
None seems correct to me, but more thinking/testing needed.
On 23 Jun 2015 10:57 am, "Thomas Leonard" [email protected] wrote:

I'm pretty sure that choose is wrong too. If one of the threads exits, we
still need to be able to cancel the others (which is the only thing we use
it for I think). Probably it should be a pick (and it would be more
efficient to use pick [a;b;c] rather than using the binary operator,
which creates two threads).

—
Reply to this email directly or view it on GitHub
#156 (comment).

[talex5]

With this patch, the test program in #153 prints this over and over again:

Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait
Tcp.State: Time_wait  - Recv_fin -> Time_wait
Tcp.State: Time_wait  - Recv_rst -> Time_wait

Here's the pcap file: http://test.roscidus.com/static/rst.pcap

[yomimono]

Thanks for the report, @talex5 - I'm really baffled by this and am not sure whether this is a strange result or my understanding of Lwt is just deficient. Any pointers appreciated.

[talex5]

Can you reproduce it? Here's a gist with my test unikernel:

https://gist.github.com/talex5/71ddeb255c2d9339d52b

[balrajsingh]

is the RST being ACK-ed? That would immediately create an RST storm.
Nothing should be sent in response to an RST. I'll look with you but can't
really do it today.

On Thu, Jun 25, 2015 at 10:29 AM, Mindy Preston [email protected]
wrote:

Thanks for the report, @talex5 https://github.com/talex5 - I'm really
baffled by this and am not sure whether this is a strange result or my
understanding of Lwt is just deficient. Any pointers appreciated.

—
Reply to this email directly or view it on GitHub
#156 (comment).

[yomimono]

@talex5 - thanks for the gist; I can reproduce. I'll squint at some traces and see how far I get.

[yomimono]

Changing choose to pick and removing the recursive call seems to be the right thing to do. (I was missing the understanding that the only reason this code was terminating originally was the exception thrown when wakeup was called the second time; thanks for your patience.)

[talex5]

So just to check: when the receive thead exits, there is nothing more for the other threads to do so they can be killed by the pick? If not (if one thread might exit while another is still running, and that's OK), we should use join.

[talex5]

Actually, join isn't right either because then if one thread crashes then we can't kill the others.

The behaviour we want (I guess) is:

• If one thread fails, report it and close the TCP connection, which will kill the other threads.
• If one thread succeeds, keep watching the others.

It might be best to keep the list of threads and do things manually, i.e.

• Attach the on_failure handler to each thread with List.iter.
• To cancel, List.iter Lwt.cancel.

[talex5]

This looks good to me! Any objections to merging?

[samoht]

LGTM

[samoht]

seems related to ocsigen/lwt#166

Do you want me to make a new release with the fix? Is there anything else we would like to include?

[talex5]

@samoht I don't think this needs a new release. It cleans up the traces by removing the error, and it reports genuine errors instead of ignoring them, but in the common case these two bugs cancel out (the thread fails instead of returning, but the error is ignored and the effect is the same). So, it will help with debugging but it doesn't fix any actual bugs by itself.