-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unexpected stateClosing during HTTP request using wget #19
Comments
Do you have access to wireshark captures of the event? It would allow us to write a test to catch this behaviour and validate the fix. |
Is there a specific format of the wireshark captures you would prefer? I could export via the export packet dissections menu, choosing text or json output. Or I could use save, choosing a format like pcapng. |
Wireshark capture files: wget_stateClosing_captures.zip
Command issued:
Source node: IP addr: 192.168.5.1, MAC: Dest node: IP addr: 192.168.5.100, MAC: |
I'll try to take a look at this tomorrow and see if I can reproduce the bug in tests. Thank you so much for putting in the effort in describing the issue in detail <3 |
@knieriem Hey so I tried adding a test that replicated the exchange but was unable to make the program panic. I did notice that the states transitioned by your program differ to the test so there's still some work to do. Could you set your logger to Here's the log of the test I wrote trying to replicate the bug:
|
Hi, please see the log -- obtained using my 10Base-T1S network -- below. [Edit: deleted a passage with an assumption that seemed possibly misleading]
|
Alright, all looks almost ready for the fix. I've just one question for you- you suggest setting the pending flag to ACK in the added switch case though after entering a TimeWait state no more data should be exchanged. Is sending out the ACK necessary in that case? I've applied your fix in #21 with a slight modification in how the pending flags are calculated. Are you able to test the fix? |
Thank you for preparing the PR. I can confirm that the fix in #21 is working properly for my setup. Regarding your question -- I agree that no ACK should be sent (this is also what the state diagrams say). [By the way, I figured out how to achieve the same message flow behaviour in my setup as in the original, cyw43439 based http-server example: Calling HandleEth more often will change the timing, so that not Closing state, but FinWait2 state is entered. Therefore a way to reproduce the behaviour involving the Closing state on the Pico W could be to call HandleEth slightly less often.] Since I looked a lot at the wireshark capture logs the last days, I began to wonder why FinWait2 is even entered in the normal, not panic resulting message flow, and why TimeWait isn't entered directly from FinWait1. From my understanding in this particular case an ACK for a previous data segment (that happens to be received just after the server sent its FIN+ACK) is mistaken for a single ACK of the server's FIN+ACK, so that FinWait2 is entered instead of waiting for a matching FIN/ACK segment. If you don't mind, I would describe this in a separate issue. |
This sounds like it could cause issues going forward. Please do create a new issue detailing observations. Logs welcome! As for the PR will merge and close this issue by end of week if nothing else crops up. |
Using
wget
to request a page from a version of your http-server example adapted for T1S,I found that at the end of the wget communication a Closing state is entered,
that will result in a panic within a
switch
statement inControlBlock.Recv
.Apparently the condition
seg.ACK == tcb.snd.NXT
ingithub.com/soypat/[email protected]/control.go:245
is not fulfilled in this case.When using a browser instead of wget, I don't observe the Closing state: In this case, a direct
transition from FinWait1 to TimeWait can be seen:
I added the lines
-- based on a TCP state diagram -- to the switch statement in
ControlBlock.Recv
, which results in the following message flow:Not sure if the added lines are sufficient.
The text was updated successfully, but these errors were encountered: