nodemcu / nodemcu-firmware

Lua based interactive firmware for ESP8266, ESP8285 and ESP32
https://nodemcu.readthedocs.io
MIT License
7.67k stars 3.13k forks source link

Fetching multiple URLs via http.get does not work #1344

Closed oyooyo closed 8 years ago

oyooyo commented 8 years ago

I want to HTTP GET multiple URLs via http.get. Since http.get does not allow concurrent requests, I believed the correct way to do this would be to start the second http request in the callback function of the first http request, as I expected the first http request to be completed when the callback function is being called. However, it seems that when doing so, the callback function of the second http request is never actually being called.

Is this an actual bug, or am I just doing http.get wrong? If so, how do I properly http.get() multiple URLs?

Expected behavior

Output:

x2    200 "AAPL"
x3    200 "MSFT"

Actual behavior

Output: x2 200 "AAPL"

Test code

http.get('http://download.finance.yahoo.com/d/quotes.csv?s=AAPL&f=s', nil, function(status_code1, body1)
  print('x2', status_code1, body1)
  http.get('http://download.finance.yahoo.com/d/quotes.csv?s=MSFT&f=s', nil, function(status_code2, body2)
    print('x3', status_code2, body2)
  end)
end)

NodeMCU version

Latest dev

Hardware

NodeMCU Devkit

oyooyo commented 8 years ago

Update: I just figured out that it works if I wrap the inner call to http.get() with tmr.alarm(), so the second call to http.get is not called directly inside the callback of of the first http.get code, but instead triggered by a timer event being registered inside the callback of the first http.get:

http.get('http://download.finance.yahoo.com/d/quotes.csv?s=AAPL&f=s', nil, function(status_code1, body1)
  print('x2', status_code1, body1)
  tmr.alarm(0, 1, tmr.ALARM_SINGLE, function()
    http.get('http://download.finance.yahoo.com/d/quotes.csv?s=MSFT&f=s', nil, function(status_code2, body2)
      print('x3', status_code2, body2)
    end)
  end)
end)

While this works, I'm not entirely happy about it since it uses an additional timer.

TerryE commented 8 years ago

Use node.task.post(). This isn't bug but a characteristic of the non-OS SDK, because it is non-preemptive. You have to split some TCP functions across callback invocations to allow other related processing to happen. The task.post() function is the easiest way to do this. The context switch only adds a few 10s µSec and you need to do this anyway :smile:

oyooyo commented 8 years ago

Thanks @TerryE , that works fine!

marcelstoer commented 8 years ago

Note to self: add a hint about task.post() to the note at the top of https://nodemcu.readthedocs.io/en/dev/en/modules/http/

oyooyo commented 8 years ago

Note to self: add a hint about task.post() to the note at the top of https://nodemcu.readthedocs.io/en/dev/en/modules/http/

I agree, this would be helpful. This probably not only affects http.get(), and the solution isn't obvious unless one is already aware of task.post().

BTW: Is there already an issue in the issue tracker collecting errors and suggestions for improvement in the documentation? If not, it might be a good idea to start one. For example, while reading the documentation I came across one or two small typos/broken links. Nothing serious, so it would have been rather ridiculous to file an own issue for it; but if there was a issue for collecting such minor errors in the docs, I could have left a short note about it there.

marcelstoer commented 8 years ago

I disagree as such an issue would never be resolved. People would keep adding new stuff and discussions (about the non-obvious issues) would make following that issue a nightmare.

marcelstoer commented 8 years ago

GitHub allows in-place editing of files. It automatically clones the repository and creates PRs behind the scenes. Feel free to keep 'em coming, those PRs 😉