-
-
Notifications
You must be signed in to change notification settings - Fork 348
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
HTTP client fixes #2163
HTTP client fixes #2163
Conversation
mikee47
commented
Dec 1, 2020
•
edited by slaff
Loading
edited by slaff
- Improvements to pipelining.
- Improvements to retrying the current request(as is the case for URLs protected with digest authentication).
- Fixed post requests sending files and/or form data.
- Small improvements to the handing of failed writes in TcpConnection.
@slaff Looks fine, works well! Thank you. |
60ac778
to
4188f5c
Compare
aa86117
to
167acf7
Compare
@@ -60,6 +60,7 @@ class MultipartStream : public MultiStream | |||
Producer producer; | |||
BodyPart bodyPart; | |||
char boundary[16] = {0}; | |||
bool finished = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shadows MultiStream::finished...
@mikee47 While trying to make the HttpClient work again I have noticed that the LimitedMemoryStream was broken in this PR #2119. The changes in the isFinished(), given below bool isFinished() override
{
return (readPos >= capacity);
} cause endless loop when trying to copy the data:
called from here: int onDownload(HttpConnection& connection, bool success)
{
debugf("\n=========[ URL: %s ]============", connection.getRequest()->uri.toString().c_str());
debugf("RemoteIP: %s", connection.getRemoteIp().toString().c_str());
debugf("Got response code: %d", connection.getResponse()->code);
debugf("Success: %d", success);
if(connection.getRequest()->method != HTTP_HEAD) {
Serial.print(_F("Got content: "));
auto stream = connection.getResponse()->stream;
if(stream == nullptr || stream->available() == 0) {
Serial.println("EMPTY!");
} else {
Serial.copyFrom(stream); IMHO the isFinished method here should be just: bool isFinished() override
{
return (available() == 0 );
} @mikee47 Can you create a separate PR with a fix? |
6b1a73c
to
25133a0
Compare
@@ -315,6 +315,7 @@ void HttpClientConnection::onClosed() | |||
System.queueCallback([this]() { | |||
cleanup(); | |||
init(HTTP_RESPONSE); | |||
state = eHCS_Ready; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be done in init()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be done in init() ?
Yes, indeed. I will add this correction in a second...
50003ab
to
feb57db
Compare
@mikee47 There is a problem with SSL and sending data in the TcpConnection. I will address also this but it will be part of a separate PR. |
There appear to be a couple of things at play here: 1. Need to move items off execution back onto waiting queue - cleanup() does this 2. Don't restart immediately, but defer using task queue - let's TCP stack sort itself out first
…s that don't support keep-alive connections. Initial conditions ------------------ 3 consecutive GET requests + 1 POST request go to the same HTTP server. Before this change ------------------ The HTTP client starts using HTTP pipelining (https://en.wikipedia.org/wiki/HTTP_pipelining) without making sure that the HTTP server is supporting it. It will try to send the first 3 HTTP requests in one go. If the HTTP server does not support pipelining it will either return bad request response or a normal response with Connection: close header. The first request will be successful but then the connection will be closed and the remaining two requests will be moved back to the waiting queue. They will be AFTER the POST request which is a **problem**. Once the POST request has completed the two GET requests will be moved back and forth between the waiting and execution queue which is another **problem**. After this change ----------------- The first GET request will be sent but the others will wait in the waiting queue. Once the request has completed the Connection response header will be analyzed. a) If the header is Connection:close then the connection will be closed and there will be no need to move something from the execution queue back to the waiting queue as there will be 0 requests in execution and the rest will be in the waiting queue. b) if there is no Connection:close header in the response the HTTP client will assume that the HTTP server supports pipelining and will allow pipelining consecutive requests in the same Http Client Connection. This way the remaining 2 GET requests will be pipelined and the POST request will be executed at the right time.
…n empty string after the extractAt call.
…in wrong multipart boudary headers and endless loop. This commit removes the introduced problems.
Add BodPart::bool() operator as it provides opportunity to clarify how it's used Rename `finished` -> `footerSent`
feb57db
to
3116ba0
Compare
Retrying of the current request will keep its order and not get at the end of the execution queue. Fixes a problem with requests that go to URLs protected with digest authentication (requires the same request to be sent twice - first time without authentication headers to get the tokens and second one with authentication and calculated checksums.
3116ba0
to
ec22769
Compare
@slaff I got the failed requests again, so re-tested with the previous working commit and still failing! So I increased the scanner I then revisited the issue wth using the task queue in ControlPoint - https://github.com/mikee47/Sming-UPnP/blob/3e66a17cbd9c9287485a4a2b177a79a0744d2f64/src/ControlPoint.cpp#L211 onwards. The issue persists, and is definitely related to this issue. I need to improve the debug output so requests can be correlated with responses more easily, then perhaps the logs will shed more light on exactly what's happening... |
So at present we can retry sending headers but the body stream also needs to be recovered. I started attempting to fix that by restoring and resetting the stream (see commit for Is there some way we can pre-empt all this and avoid starting a new request if the connection is due to be closed? |
36bb982
to
198b488
Compare
We have to analyze more carefully the problem reported in SmingHub#2158 and issue a separate PR with the fix.
198b488
to
4b61a3e
Compare
@mikee47 Once Travis is ready I will merge this PR in develop. It does not solve your problem but at least has some useful fixes. Any idea how I can reproduce the problem that your experiencing on my machine? This will help me go step by step with the debugger and correlate network traffic with live wireshark capture. |
@slaff I got the same issue running the Basic_ControlPoint sample on an ESP8266. I'll re-check to confirm and get back to you. |
Get the latest develop and rebase on it. Compile your sample using the following commands (adjust accordingly ):
|
See #2173 . |
@slaff I've rebuilt and re-tested as follows:
The attached log I then checked out the
The results are in When a device has been discovered and created by ControlPoint, the client callback creates action requests and submits them. The action requests are for
You could probably run MiniDLNA on another device to check this - seems very unlikely it's the router which is the problem but I'll see if I can find an alternative device to test this against. |