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

Fix data loss in TCP streams. #2097

Merged
merged 2 commits into from
Sep 28, 2017
Merged

Conversation

jmattsson
Copy link
Member

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • [n/a] The code changes are reflected in the documentation at docs/en/*.

So it turns out that very occasionally^ the lwip stack hands over chained pbufs to the tcp_recv() callback. As only the first pbuf in the chain was passed into Lua, this meant that data went missing from the middle of the TCP stream (which one would certainly not expect). The fix here is to simply do multiple callbacks into Lua until the end of the chain is reached (though I've never seen more than two pbufs in the chain so far).

With this patch I'm now always successfully getting the full TCP stream, whereas before I never did.

^) During each 1meg transfer, I saw it happen 1-4 times.

@jmattsson jmattsson requested a review from djphoenix September 8, 2017 07:12
@marcelstoer marcelstoer added this to the 2.1 follow-up II milestone Sep 8, 2017
Copy link
Member

@devsaurus devsaurus left a comment

Choose a reason for hiding this comment

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

Looks good 👍

num_args += 2;
char iptmp[16];
bzero(iptmp, 16);
ets_sprintf(iptmp, IPSTR, IP2STR(&addr->addr));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be moved outside a loop

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though it becomes (slightly) harder to follow, and you double up on the if statement as you'll need to push the port/ip each time through the loop regardless. Given that we'd normally be doing the ets_sprintf for each pbuf anyway, I felt the tradeoff wasn't worth the readability cost. If you really reckon it is, I can change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree for "harder to follow"...
Will accept 'FIXME' comment there as a solution for now.

@marcelstoer marcelstoer merged commit 3e60fa8 into nodemcu:dev Sep 28, 2017
@joysfera
Copy link
Contributor

joysfera commented Oct 8, 2017

wondering if this patch could fix also the mysterious MQTT issues (where bits of TCP data were missing randomly) #1811

@jmattsson
Copy link
Member Author

Afaiu mqtt uses espconn, so this shouldn't be applicable there. Someone is welcome to prove me wrong though :)

crasu pushed a commit to crasu/nodemcu-firmware that referenced this pull request Jan 11, 2018
* Fix data loss in TCP streams.

* Factored out the UDP extra args handling.
jmattsson pushed a commit to DiUS/nodemcu-firmware that referenced this pull request Aug 20, 2018
* Fix data loss in TCP streams.

* Factored out the UDP extra args handling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants