Skip to content
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

ESP8266 TCP - connection closure error #4

Closed
wants to merge 1 commit into from

Conversation

quozl
Copy link
Collaborator

@quozl quozl commented Sep 19, 2016

According to stack effect comment and lwip.c/recv_cb, connection closure in receiver should return something. Tested with returning ERR_ABRT, seems to work fine. Perhaps some other value?

Also adjust comment for tcp-recved, which opens the TCP window by accounding for the bytes handled by the application.

(A local test application of mine calls tcp-recved before handling data.)

See 53151b3.

According to stack effect comment and lwip.c/recv_cb, connection closure
in receiver should return something.  Tested with returning ERR_ABRT,
seems to work fine.  Perhaps some other value?

Also adjust comment for tcp-recved, which opens the TCP window by
accounding for the bytes handled by the application.

(A local test application of mine calls tcp-recved before handling
data.)
@@ -75,6 +75,7 @@ defer respond ( pcb -- close? )
?dup 0= if ( pbuf )
." Connection closed" cr cr
rx-pcb close-connection ( )
ERR_ABRT
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that ERR_OK is appropriate here. This is not a premature abort, but rather a normal shutdown.

That said, looking at the code in lwip/core/tcp_in.c, there is very little difference between the code paths in the two cases. The only difference is that, in the OK case, tcp_output() is called before falling through to the aborted: label. I think that is a good thing, as it permits the stack to proceed apace with the final stages of the connection close dance.

Regarding the timing of tcp_recved versus handle-data, it's unclear to me what is correct. On the one hand, calling tcp_recved early would potentially speed things up by getting the TCP ACK out sooner. On the other hand, it seems prudent to make sure the data is safe before proceeding. On the other other hand, the data is probably safe until the pbuf is freed.

Analysis of tcp_in.c suggests that, before the pbuf is passed to the application, it is disconnected from the pcb, so it should be safe to call tcp_recved before handling the data. That being the case, I have moved the tcp-recved call up to before handle-data. We can keep our fingers crossed.

Copy link
Owner

@MitchBradley MitchBradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I incorporated the spirit of your patch, with some changes and elaborated commentary. See the in-code comments for more details.

@quozl
Copy link
Collaborator Author

quozl commented Sep 19, 2016

Thanks. Reviewed 322fa90 and found it good.

@quozl quozl closed this Sep 19, 2016
@quozl quozl deleted the fix-tcp-stack-effects branch September 19, 2016 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants