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

Multiple conn:send() not working #730

Closed
positron96 opened this issue Nov 5, 2015 · 25 comments
Closed

Multiple conn:send() not working #730

positron96 opened this issue Nov 5, 2015 · 25 comments

Comments

@positron96
Copy link

When I was on a stable build (0.9.6-dev_20150704), the code like this worked correctly:

    connection:on ("receive",
    function (conn, req)
        conn:send("HTTP/1.0 200 OK\n")
        conn:send("Server: ESP (nodeMCU)\n")
        conn:send("Content-Length: 1\n\n")
        conn:send("a")
        conn:close()
    end )

This worked for a client socker as well as a server socket.

Now I moved to dev build that says:

NodeMCU custom build by frightanic.com
    branch: dev
    commit: d8c6fe0b25142f6ae866c18c3bcb3d0538d4baaf
    SSL: false
    modules: node,file,gpio,wifi,net,pwm,tmr,uart,ow
 build  built on: 2015-11-03 20:47

And only first conn:send actually sends, all following calls get silently discarded. I now need to store the lines into a table and join table into a string which is probably a waste of memory.

Is it a bug or is it intended?

@TerryE
Copy link
Collaborator

TerryE commented Nov 6, 2015

The outcome of your code is indeterminate as network requests are treated as separate tasks by the SDK. Ignoring the fact that you should marshal your sends where practical, something like this would work:

  connection:on ("receive",
    function(sck, req)
      local response = {"HTTP/1.0 200 OK", "Server: ESP (nodeMCU)", "Content-Length: 1\n", "a"}
      local function sender (sck)
        if #response>0 then sck:send(table.remove(response,1))
        else sck:close()
        end
      end
      sck:on("sent", sender)
      sender(sck)
    end )

One network call per task and no ordering problems. And yes this is could be viewed as bad coding to have two semantically different variables both called sck but in reality they both point to the same userdata. This really falls under #719 unless this also fails, but I'll give you a chance to reply first 😄

In reality, I'd never encode it this way because your algo implicitly assumes that you'll get a bare request and this rarely happens. HTTP 1.1 is a complex protocol yet everone using it seems to make a load of dumb assumptions and then wonders why their code is flacky.

@jmattsson
Copy link
Member

Is it a bug or is it intended?

This is a result of changes within the SDK. Pre-1.x the SDK seemed willing to buffer up contents from several send() calls despite it not being officially supported, but that is no longer the case. I suspect (but haven't verified) that this happened when they freed up substantial amounts of RAM for application use.

So well, this is neither a bug nor intended, it's simply what's being imposed by the SDK.

@TerryE TerryE closed this as completed Nov 6, 2015
@positron96
Copy link
Author

Hello Terry. The code you provided works, as does concatenating the table into string, but sending a table with recursive ''''sent'''' events is not very intuitive to say the least. I'll sacrifice memory and stick with table concatenation as more clear way.
Also, could this behavior be documented anywhere? I believe I'm not the only one trying to send data with several successive calls.
(Btw, my example code is not the whole piece of http request handler of course:-)

@TerryE
Copy link
Collaborator

TerryE commented Nov 6, 2015

@positron96 read my Unofficial NodeMCU FAQ. It really need a major update but it does explain this sort of issue. As Johnny says, this was never supposed to work but it did on 0.9x unless you died on memory and PANICed

@dwery
Copy link

dwery commented Nov 10, 2015

This seems to be a common issue, it happened to me too as it worked on 0.9.5. maybe the http example or documentation could be amended or a better example provided. I resorted to concatenating a couple of strings but it's not very classy,

@nanodeath
Copy link

Even if this is "as intended", it's still confusing as a new user. I've added a new Note section to the docs, which ultimately links back to this page; feel free to update it if it needs it.

@makefu
Copy link
Contributor

makefu commented Jan 11, 2016

@TerryE based on your response i tried to write a bit larger file (> 1024bytes) but i have the issue transferring the file in a chunked manner because file IO is slower than the socket IO.

What is the nodemcu way to solve this issue?

@Hellsy22
Copy link

Why not fix this on firmware side? I mean let's return the queue.
Most of languages care about sending data to socket (syswrite, socket.write, print SOCKET etc) - for user it working just like a black box.

@TerryE
Copy link
Collaborator

TerryE commented Jan 23, 2016

@Hellsy22, Espressif broke backwards compatibility because the 0.9x approach burned memory resources and caused all sorts of instabilities. We don't see it as our job to undo sensible SDK changes. The SDK is task-driven, not procedural. We can't change this.

@Hellsy22
Copy link

"We don't want to burn memory resources by queueing send on SDK side, so we force all users to burn twice more resources by queueing at the LUA side". Ok, I got the point.

But what if I'll make pull-request with a new method called "qsend" working as old fashioned send? Will it be ok or still unaccepable?

@TerryE
Copy link
Collaborator

TerryE commented Jan 23, 2016

Why bother? Replicating the old functionality will be really complex and memory intensive. Getting it right is going to be difficult. It's just so much easier to code up your Lua correctly in the first place.

@Hellsy22
Copy link

Because simplicity is the one of a greatest NodeMCU's benefits. Last time I checked project page it was "Easy to program". Not "Will teach you how to code up your Lua correctly and then will break reverse compatibility to teach you again and again".

So, will do it anyway. As a lib on Lua side or as a PR for firmware. I prefer the PR, because it's more effective, but if it's unacceptable - just say it, please.

@TerryE
Copy link
Collaborator

TerryE commented Jan 23, 2016

There are SDK architectural reasons why this will be almost impossible. With the 1.4+ SDK not only can you only issue one send in a task, you can't issue a close either. Even if you implement an I/O queue in your library, you will need to queue all I/O operations. You have no control over the order of tasks firing, so if you do a couple of sends then have a timer alarm to close the socket, then you have no guarantee that the two sends will be scheduled before the alarm delivers the cb to close the socket. So by all means try, or alternatively wait for the ESP32 and RTOS and port eLua to that.

@Hellsy22
Copy link

@TerryE, but why it can be almost impossible? SDK not supporting queue? Ok, it can be created on nodemcu side: just add a few lines of code to net_send to set flag "sending" or put request to queue of flag already set; to net_socket_sent - to callback net_sent if queue not empty or do lua callback if all sent. And a few more to net_close - to wipe the queue. Adding an additional functions like classic "flush", "shutdown" or "flush_and_close" - will be great.

It can be done on Lua side too, but by very ugly way - I have no idea how to override functions from romtable (may be you know how?), so it will be like: myNet.send( socket, data), myNet.close( socket ), myNet.on( socket, name, callback) etc.

So, I do not coding C for an about 15 years, but still can do it. All questions are - are there the better way and will it be accepted?

@TerryE
Copy link
Collaborator

TerryE commented Jan 24, 2016

Coding up a task driven approach in Lua is straight forward. Feel free to develop what you want, and by all means issue a PR, but: it has to work and be bug free; it must not interfere with correct SDK functionality; it must not create resourcing issues that will make normal operation unstable; there must be a consensus that general users will want this. I personally think it extremely unlikely that you achieve all these goals.

@TonyMony
Copy link

Hello everybody,
it seems we will not have a nodemcu version that works old way of file sending. In this case I'm wondering is there any working lua example for sending file chunk by chunk with new SDK?

@TerryE
Copy link
Collaborator

TerryE commented Jan 24, 2016

Just raised #971 to cover this.

@bixente57
Copy link

Hi,

I have two issues trying to implement your solution but for a on:connection event.

Firt issue : I get a "attempt to call field 'remove' (a nil value))" error when calling table.remove. This exact same call is OK in a separate sandbox file. Maybe this is a scope problem.

Second issue (main issue) : this code doesn't work:

conn:on("connection", function(sck, payload)
      local t_req = truncateString(req, 500)
      reverse(t_req)
      local function sender (sck)
        if #t_req>0 then sck:send(t_req[#t_req]) t_req[#t_req]=nil
        else sck:close()
        end
      end
      sck:on("sent", sender)
      sender(sck)
    end)

truncateString returns a table with small packets

I get no http response to this.

The request is OK, I've tried successfully to send it to httpbin.org in one piece. The problem happens only when I try to send in multiple parts.

@dvv
Copy link
Contributor

dvv commented Jan 28, 2016

Well we discuss pure Lua stuff which can be modelled on host computer.
The whole example is not seen.

@seblanc
Copy link

seblanc commented Mar 18, 2016

Dear all, I am looking for a working example to how to read and send multiple html files with the new SDK for a basic webserver. I know this is not the right place to ask, so I opened a question on StackOverflow. Hopefully this could help other people..!

http://stackoverflow.com/questions/36079145/how-to-send-multiple-data-connsend-with-the-new-sdk-nodemcu

@RinusW
Copy link

RinusW commented Mar 25, 2016

One could solve these issues quite elegantly by using the send(msg, function(cb)) function call in stead of using the :on("send") method. For example, the next code snap (from positron96's first posting )

    connection:on ("receive",
    function (conn, req)
        conn:send("HTTP/1.0 200 OK\n")
        conn:send("Server: ESP (nodeMCU)\n")
        conn:send("Content-Length: 1\n\n")
        conn:send("a")
        conn:close()
    end )

could elegantly reformulated as follows:

   connection:on ("receive",
    function (conn, req)
        conn:send("HTTP/1.0 200 OK\n", function(c1)
          c1:send("Server: ESP (nodeMCU)\n", function(c2)
          c2:send("Content-Length: 1\n\n", function(c3)
          c3:send("a", function(c4)
          c4:close() end) --close c4
          end) --close c3
          end) --close c2
          end) --close c1
    end )

It looks like I'm using function-in-function calls, but these are not normal functions. These are callbacks that get executed when the accompanying send function has been executed. So, the form of writing is function-in-function, but the execution order is completely different.
Based on the same principle I also developed a function that sends out a webpage (htlm?) to a webclient. But you will find similar solutions in the above reference to the StackOverflow page.

@TerryE
Copy link
Collaborator

TerryE commented Mar 25, 2016

@RinusW Rinus, this might be elegant but on a memory-limited system like the ESP, this is an unwise implementation.

@pastukhov
Copy link
Contributor

@TerryE Can you give a short example how it should be?

@TerryE
Copy link
Collaborator

TerryE commented Mar 25, 2016

OK. I'll bump this on my TODO list, but I want to get #1168 and #1119 out of the way first.

marcelstoer added a commit to marcelstoer/nodemcu-firmware that referenced this issue Mar 25, 2016
@pastukhov
Copy link
Contributor

Thanks @marcelstoer

marcelstoer added a commit to marcelstoer/nodemcu-firmware that referenced this issue Mar 29, 2016
marcelstoer added a commit to marcelstoer/nodemcu-firmware that referenced this issue Mar 29, 2016
wtarreau added a commit to wtarreau/iot-core that referenced this issue Feb 3, 2020
After upgrading the modules to version 1.5.4, the telnet server stopped
working. This issue is described in details in this comment :

  nodemcu/nodemcu-firmware#730 (comment)

In short the problem is that net.socket:send() doesn't implement EAGAIN
and used to buffer everything and to cause memory outages. Now it doesn't
do that anymore and it's up to the application to do it instead (which is
not much better). The issue above suggests that implementing a local buffer
and manipulating it via table.insert() and table.remove() is the most optimal
solution avoiding expensive memory copies. This is what this commit does.
Please note that table.insert() is misnamed, as it appends in reality.
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

No branches or pull requests