Skip to content
This repository has been archived by the owner on Aug 4, 2020. It is now read-only.

get(feed, xivelyKey); call takes 1 minute to return with the data. #1

Open
nik8989 opened this issue May 16, 2013 · 26 comments
Open

get(feed, xivelyKey); call takes 1 minute to return with the data. #1

nik8989 opened this issue May 16, 2013 · 26 comments

Comments

@nik8989
Copy link

nik8989 commented May 16, 2013

I was running the DatastreamDownload.ino example with just my key and feed as the only modifications to the code. and it was taking 1 minutes to run this line of code:

int ret = get(feed, xivelyKey);

I did some debugging and found that the implementation of xivelyclient.get() in XivelyClient.cpp was hanging in the following while loop:

while ((next != '\r')  && (next != '\n') && (http.available() || http.connected()))
{
   next = http.read();
}

I imagine that the only reason it was ever coming out of this loop is because the connection is being closed by the server.

In order to make the function work for me I added the last two lines in the if statement just above the while loop and deleted the while loop.

if ((idBitfield & 1<<i) && (aFeed[i].idLength() == idIdx))
{
   // We've found a matching datastream
   // FIXME cope with any errors returned
  aFeed[i].updateValue(http);

  // When we get here we'll be at the end of the line, but if aFeed[i]
  // was a string or buffer type, we'll have consumed the '\n'
  next = '\n';
  http.stop();
  return ret;
}

I'm sure this is not an elegant solution but it works for me for now...

BTW thanks for writing this wrapper. It works for everything else I've tried.

@errordeveloper
Copy link
Contributor

Looks like the same as cosm/cosm-arduino#12

@nik8989
Copy link
Author

nik8989 commented May 16, 2013

Agreed. Issues are identical. Looks like a more elegant solution too.

@errordeveloper
Copy link
Contributor

Since the old issues are no longer accessible, here is the solution proposed by @zerohuit:

-while( ( next != '\r' )  && ( next != '\n' ) && ( http.available() || http.connected() ) )
+while( ( next != '\r' )  && ( next != '\n' ) && ( http.available() && http.connected() ) )

@nik8989
Copy link
Author

nik8989 commented May 16, 2013

Hi Ilya,

Is this a real bug in your opinion? If so, why hasn't the code been updated?

Nick

On 17/05/2013, at 12:35 AM, Ilya Dmitrichenko notifications@github.com wrote:

Since the old issues are no longer accessible, here is the solution proposed by @zerohuit:

for me, we should have while ((http.available() && http.connected())) instead while ((http.available() || http.connected()))


Reply to this email directly or view it on GitHub.

@errordeveloper
Copy link
Contributor

@nik8989 this fix will need testing. Have you tried that fix with replacing || by &&?
I haven't had the time to test it myself yet, might be able to try tomorrow...

@nik8989 nik8989 closed this as completed May 16, 2013
@nik8989
Copy link
Author

nik8989 commented May 16, 2013

@errordeveloper I did try replacing the || with && and that fix worked also. I haven't done extensive testing but the fix looks right to me.

@errordeveloper
Copy link
Contributor

Don't close, the upstream code isn't fixed yet. I'll close once the fix is applied here.

@roquito
Copy link

roquito commented May 19, 2013

Hi, for all us new to this out there, what do we do? is there an updated library that we can download? or can someone post a good write up on how to solve this? Thanks in advance!

@errordeveloper
Copy link
Contributor

@roquito @nik8989 I have just pushed a potential fix for this, could you please test and see if it works for you?

You can either checkout branch issue-1-fix-loop-logic or download an archive.

I have chaged both (inner and outer) loop conditions, please let me know if this works for you. It's possible that only one of the condition needs to be change. Please test the fix an comment back!

@errordeveloper
Copy link
Contributor

@amcewen, would you mind taking a quick look at this also?

@roquito
Copy link

roquito commented May 19, 2013

Trying it now... stand by

BTW, tried editing the .cpp file per the above comment (just replacing || with &&) and did not make a difference but I not sure if all I had to do was just edit the file, save it and upload to Arduino (?).

@roquito
Copy link

roquito commented May 19, 2013

@errordeveloper you got it man. It works. Thanks for helping the new members out.

@lwotton
Copy link

lwotton commented May 29, 2013

@errordeveloper Replacing II with &&, worked here partially, was receiving constant -4 response codes from the client before I switched it however data values are no longer received and just 0.00 comes through from a .getFloat()

@amcewen
Copy link
Contributor

amcewen commented May 29, 2013

Ignore the referenced issue in arduino/Arduino. I thought that was the problem at first. but this code isn't using the Stream::find methods at that point, so it won't be the problem.

Replacing the || with && isn't going to fix things properly, as it means the code can drop out early if there's a pause in the data being delivered from the server.

I can't reproduce the problem here, I'm wondering if it's something caused by transparent HTTP proxies sat in the middle keeping the connection open for too long? Does anyone who's seen the problem happen to know if there's a proxy in between them and Xively?

One thing that might improve matters would be to add a check for the stream being completed (this uses the Content-Length header to work out if it's hit the end of the page being downloaded)

-while( ( next != '\r' ) && ( next != '\n' ) && ( http.available() || http.connected() ) )
+while( ( next != '\r' ) && ( next != '\n' ) && !http.completed() && ( http.available() && http.connected() ) )

It still seems to work when I test it, but given that I haven't seen the delay I don't know if it's fixed anything...

@lwotton
Copy link

lwotton commented May 29, 2013

Using that while statement plus the issue-1-fix-logic-loop has given me solid 200 returns from Xively - the delay has dissapeared.

Tracert here

Although not a single value comes through, Xively reports it as a 200 with the correct values on my dashboard but Arduino fails at picking any of them up and .GetFloat just reports 0.00 on all streams. Hmm

@errordeveloper
Copy link
Contributor

@amcewen I'm not quite sure what you meant on line 110:

// FIXME Need to time out if this hangs for too long

Could it be related? That was my first thought actually...

@errordeveloper
Copy link
Contributor

@amcewen it feels like that HttpClient should return a pointer to a buffer where the entier body is stored... even if this is this processing chunked response, it should still only try to parse after entire payload after it had been received. It's not using Content-Length header as I can see, and that doesn't feel quite right.

@amcewen
Copy link
Contributor

amcewen commented May 30, 2013

@errordeveloper the comment on line 110 wouldn't cause this, or at least, if it does I'd still want it to wait for longer than 1 minute before it does gives up :-) It's possible in that part that if the remote server hangs without closing the connection then the client could also hang, so it should get fixed. However, you'd usually have a timeout of nearer to five minutes (or more) - it would be catching a non-normal situation, rather than a standard behaviour

@amcewen
Copy link
Contributor

amcewen commented May 30, 2013

@errordeveloper we can't store the entire body in HttpClient because we'd easily blow the RAM budget on Arduino. One of my scratchpad test feeds - 15552 - is over 1KB of data returned, which would be over half the RAM available in an Uno.

The HttpClient code will extract the content length if it encounters the header (either while calling readHeader() or skipResponseHeaders()). HttpClient::read will (at present, I can argue both sides as to whether it should or not) let you try to read past the end of the body, but HttpClient::endOfBodyReached will report true when you've consumed Content-Length bytes of the body.

@errordeveloper
Copy link
Contributor

@amcewen true, your feed gives 1092 in JSON, but it's only 303 in CSV, but you could pass datastreams= to it like so:

> GET /v2/feeds/15552.csv?datastreams=humidity,random_string HTTP/1.1
> User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5
> Host: api.xively.com
> Accept: */*
> X-ApiKey: ...
> 
< HTTP/1.1 200 OK
< Date: Thu, 30 May 2013 21:14:01 GMT
< Content-Type: text/plain; charset=utf-8
< Content-Length: 110
< Connection: keep-alive
< X-Request-Id: 1a56492f724fa9d4f54fb203010cbcd246cfdf90
< Cache-Control: max-age=86400
< Last-Modified: Thu, 30 May 2013 21:14:01 GMT
< Vary: Accept-Encoding
< 
humidity,2013-05-30T21:14:01.191988Z,hello0.00
random_string,2012-11-29T19:04:08.022712Z,58 is a random number

That would work pretty well, especially because the library only does static allocations as far as I can tell:

XivelyDatastream datastreams[] = {
  XivelyDatastream(temperatureId, strlen(temperatureId), DATASTREAM_FLOAT),
};
XivelyFeed feed(15552, datastreams, 1);

Therefore it will actually discard most of the datastreams it have received from this particular feed you have mentioned.

@Scott216
Copy link

Where can I find the version of XivelyClient.cpp that fixes this issue? is it this file:
https://raw.github.com/xively/xively_arduino/issue-1-fix-loop-logic/XivelyClient.cpp

@roquito
Copy link

roquito commented Oct 18, 2013

@Scott216 just download the entire library from the link that @errordeveloper posted above in this same thread about 5 months ago. It will contain the corrected file.

@Scott216
Copy link

@errordeveloper
Copy link
Contributor

Yes, that's the one. You could also fetch the XivelyClient.cpp aa you
suggested earlier :+1
On 18 Oct 2013 04:26, "Scott216" notifications@github.com wrote:

Is this the link you're talking about:
https://github.com/xively/xively-arduino/archive/issue-1-fix-loop-logic.zip


Reply to this email directly or view it on GitHubhttps://github.com//issues/1#issuecomment-26570000
.

@amcewen
Copy link
Contributor

amcewen commented Nov 25, 2013

Okay, have seen this problem now - not sure why I wasn't seeing it earlier.

The problem is that the Xively server isn't closing the connection, so the client code is waiting around in case there's more data to process. When the Xively server times out and closes the connection, then everything proceeds through and you get the reply.

The reason the server isn't closing the connection is because the client doesn't send the "Connection: close" HTTP header. Once you send that, it completes much more quickly.

I've pushed the fix into release 2.1 of the HttpClient library, so if you update to that it should fix things.

@muhammadsatrio
Copy link

Hi,
i still have different value in my Serial Monitor and my Xively
here,
xivelyclient.get returned 200
Datastream is...
{ "id" : "SUHU", "current_value" : "0.00" }

but value in my Xively is 280

i have using library from
https://github.com/xively/xively-arduino/archive/issue-1-fix-loop-logic.zip
and HTTPClient from
https://github.com/amcewen/HttpClient

and the return value from http.available() is 0

actually i have successfully to retrieve data from xively, but only 4 times successful of 71 trials

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants