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.

Sign up for GitHub

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

Refactor Web Server Parsing-impl.h using StreamString to avoid String Reallocations #9005

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor Web Server Parsing-impl.h using StreamString to avoid String Reallocations #9005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ae9c5b
reserve the String Length in urlDecode to avoid String Reallocations
Lan-Hekary Oct 16, 2023
57aaf27
replace Strings with with StreamStrings to avoid String Reallocations.
Lan-Hekary Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions cores/esp8266/Stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

#include <Arduino.h>
#include <Stream.h>
#include <StreamString.h>

#define PARSE_TIMEOUT 1000 // default number of milli-seconds to wait
#define NO_SKIP_CHAR 1 // a magic char not found in a valid ASCII numeric field
Expand Down Expand Up @@ -288,6 +289,59 @@ String Stream::readStringUntil(const char* terminator, uint32_t untilTotalNumber
return ret;
}

String Stream::readStreamString(const ssize_t maxLen ,const oneShotMs::timeType timeoutMs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is exactly the same as ::sendSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not Exactly, It constructs a String Object and a Stream Object attached to it, then populate it, then returns the String Object, it's behavior is more like ::readString

`sendSize' expect you to pass a Pointer to a Stream and returns the total number of bytes.

readStreamString is just a wrapper for the String and Stream contruction.

Copy link
Collaborator

@d-a-v d-a-v Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is indeed not exactly the same.
The goal of the send* functions is to handle and accelerate transfers for any kind of stream like StreamString are.

(edited:)

line = client.readStreamString(size); // (no copy)

can also be written as

client.sendSize(S2Stream(line), size); // no copy (not tried though, meant to work, temporary is assumed)

Anyway this function is not used in the following of your pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, your version is much smaller 😄
My version is not used indeed, but made it available so that we have a Uniform API like 'readString*' but with streams.

Please advise on the best course of action, should I remove the unused functions ?
Do you have a better Idea to make the API more intuitive ?

String ret;
S2Stream stream(ret);
sendGeneric(&stream, maxLen, -1, timeoutMs);
return ret;
}

String Stream::readStreamStringUntil(const int readUntilChar, const oneShotMs::timeType timeoutMs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as ::sendUntil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on ::readStringUntil

String ret;
S2Stream stream(ret);
sendGeneric(&stream, -1, readUntilChar, timeoutMs);
return ret;
}

String Stream::readStreamStringUntil (const char* terminatorString, uint32_t untilTotalNumberOfOccurrences, const oneShotMs::timeType timeoutMs) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

For full genericity with any kind of stream, we should rather extend ::sendGeneric to accept a char* instead of a char.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We Should Do that, but I am having a hard time making this modification.
This Function will be much simpler if we can do that.
But The basic Idea here is to match the API of readStringUntil for single char in the first function and Terminator String in this function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For full genericity with any kind of stream, we should rather extend ::sendGeneric to accept a char* instead of a char.

Having tried implementing that...

  • we'd have to change API from char to cstr*?
  • 'basic' streams have to have some kind of logic jump to stall writes until delimiter is actually read, since the current one simply pushes char-by-char
  • 'peekBuffer' streams don't have the issue above, but still have to have some kind of extra state management to track delimiter

Still, original Stream methods could be improved? idk if users care about internals here, only that String is the end result.
i.e. master...mcspr:esp8266-Arduino:strings-webserver/pr9005 as a small experiment where we don't care about the type of Stream, and utilize String buffer as a window into delimited data
(which btw would extend to some other internal libs, not only webserver)

String ret;
S2Stream stream(ret);
uint32_t occurrences = 0;
size_t termLen = strlen(terminatorString);
size_t termIndex = 0;
// Serial.printf("S %s\n",terminatorString);
while(1){
size_t read = sendGeneric(&stream, -1, terminatorString[termIndex], timeoutMs);
// Serial.printf("r %d, l %d, ti %d\n", read, termLen, termIndex);
if(getLastSendReport() != Report::Success) {
Serial.printf("Error %d\n", (int) getLastSendReport());
break;
}
if(termIndex == termLen - 1){
// Serial.printf("m %d\n", occurrences);
if(++occurrences == untilTotalNumberOfOccurrences){
break;
}else{
ret += terminatorString;
termIndex = 0;
continue;
}
}
int c = timedPeek();
// Serial.printf("c %c %02X\n", c, c);
if( c >= 0 && c != terminatorString[++termIndex]){
ret += String(terminatorString).substring(0, termIndex);
termIndex = 0;
continue;
};
if(c < 0 || (read == 0 && termIndex == 0)) break;
}

return ret;
}



// read what can be read, immediate exit on unavailable data
// prototype similar to Arduino's `int Client::read(buf, len)`
int Stream::read (uint8_t* buffer, size_t maxLen)
Expand Down
4 changes: 4 additions & 0 deletions cores/esp8266/Stream.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,10 @@ class Stream: public Print {
size_t sendSize (Stream& to, const ssize_t maxLen, const oneShotMs::timeType timeoutMs = oneShotMs::neverExpires) { return sendSize(&to, maxLen, timeoutMs); }
size_t sendSize (Stream&& to, const ssize_t maxLen, const oneShotMs::timeType timeoutMs = oneShotMs::neverExpires) { return sendSize(&to, maxLen, timeoutMs); }

String readStreamString (const ssize_t maxLen = -1 ,const oneShotMs::timeType timeoutMs = oneShotMs::neverExpires);
String readStreamStringUntil (const int readUntilChar, const oneShotMs::timeType timeoutMs = oneShotMs::neverExpires);
String readStreamStringUntil (const char* terminatorString, uint32_t untilTotalNumberOfOccurrences = 1, const oneShotMs::timeType timeoutMs = oneShotMs::neverExpires);

// remaining size (-1 by default = unknown)
virtual ssize_t streamRemaining () { return -1; }

Expand Down
28 changes: 10 additions & 18 deletions libraries/ESP8266WebServer/src/Parsing-impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,8 @@ static bool readBytesWithTimeout(typename ServerType::ClientType& client, size_t
template <typename ServerType>
typename ESP8266WebServerTemplate<ServerType>::ClientFuture ESP8266WebServerTemplate<ServerType>::_parseRequest(ClientType& client) {
// Read the first line of HTTP request
String req = client.readStringUntil('\r');
String req = client.readStreamStringUntil("\r\n");
DBGWS("request: %s\n", req.c_str());
client.readStringUntil('\n');
//reset header value
for (int i = 0; i < _headerKeysCount; ++i) {
_currentHeaders[i].value.clear();
Expand Down Expand Up @@ -122,8 +121,7 @@ typename ESP8266WebServerTemplate<ServerType>::ClientFuture ESP8266WebServerTemp
uint32_t contentLength = 0;
//parse headers
while(1){
req = client.readStringUntil('\r');
client.readStringUntil('\n');
req = client.readStreamStringUntil("\r\n");
if (req.isEmpty()) break; //no more headers
int headerDiv = req.indexOf(':');
if (headerDiv == -1){
Expand Down Expand Up @@ -198,8 +196,7 @@ typename ESP8266WebServerTemplate<ServerType>::ClientFuture ESP8266WebServerTemp
String headerValue;
//parse headers
while(1){
req = client.readStringUntil('\r');
client.readStringUntil('\n');
req = client.readStreamStringUntil("\r\n");
if (req.isEmpty()) break;//no moar headers
int headerDiv = req.indexOf(':');
if (headerDiv == -1){
Expand Down Expand Up @@ -351,11 +348,10 @@ bool ESP8266WebServerTemplate<ServerType>::_parseForm(ClientType& client, const
String line;
int retry = 0;
do {
line = client.readStringUntil('\r');
line = client.readStreamStringUntil("\r\n");
++retry;
} while (line.length() == 0 && retry < 3);

client.readStringUntil('\n');
//start reading the form
if (line == ("--"+boundary)){
std::unique_ptr<RequestArgument[]> postArgs(new RequestArgument[WEBSERVER_MAX_POST_ARGS]);
Expand All @@ -367,8 +363,7 @@ bool ESP8266WebServerTemplate<ServerType>::_parseForm(ClientType& client, const
String argFilename;
bool argIsFile = false;

line = client.readStringUntil('\r');
client.readStringUntil('\n');
line = client.readStreamStringUntil("\r\n");
if (line.length() > 19 && line.substring(0, 19).equalsIgnoreCase(F("Content-Disposition"))){
int nameStart = line.indexOf('=');
if (nameStart != -1){
Expand All @@ -388,19 +383,16 @@ bool ESP8266WebServerTemplate<ServerType>::_parseForm(ClientType& client, const
DBGWS("PostArg Name: %s\n", argName.c_str());
using namespace mime;
argType = FPSTR(mimeTable[txt].mimeType);
line = client.readStringUntil('\r');
client.readStringUntil('\n');
line = client.readStreamStringUntil("\r\n");
if (line.length() > 12 && line.substring(0, 12).equalsIgnoreCase(FPSTR(Content_Type))){
argType = line.substring(line.indexOf(':')+2);
//skip next line
client.readStringUntil('\r');
client.readStringUntil('\n');
client.readStringUntil("\r\n");
}
DBGWS("PostArg Type: %s\n", argType.c_str());
if (!argIsFile){
while(1){
line = client.readStringUntil('\r');
client.readStringUntil('\n');
line = client.readStreamStringUntil("\r\n");
if (line.startsWith("--"+boundary)) break;
if (argValue.length() > 0) argValue += '\n';
argValue += line;
Expand Down Expand Up @@ -474,8 +466,7 @@ bool ESP8266WebServerTemplate<ServerType>::_parseForm(ClientType& client, const
_currentUpload->type.c_str(),
(int)_currentUpload->totalSize);
if (!client.connected()) return _parseFormUploadAborted();
line = client.readStringUntil('\r');
client.readStringUntil('\n');
line = client.readStreamStringUntil("\r\n");
if (line == "--") { // extra two dashes mean we reached the end of all form fields
DBGWS("Done Parsing POST\n");
break;
Expand Down Expand Up @@ -514,6 +505,7 @@ String ESP8266WebServerTemplate<ServerType>::urlDecode(const String& text)
char temp[] = "0x00";
unsigned int len = text.length();
unsigned int i = 0;
decoded.reserve(len - (std::count(text.begin(), text.end(), '%') * 2));
while (i < len)
{
char decodedChar;
Expand Down