Creating a buffer for parseHeader and parsePostData #622

patrickjahns · 2016-02-27T02:43:06Z

This addresses #615 and also prepares for working on multipart requests

The previous implementation did not account that header information could be split between several packets. Also post data can be split across several TCP packets.

The changes address this by storing not processed data in a temporary buffer and pre pending the new tcp buffer so it will be parsed together

Has been tested with several requests (GET/POST including Cookies and large headers)

patrickjahns · 2016-03-01T10:58:50Z

@hreintke

any comments so far?

PTDreamer · 2016-03-01T14:10:12Z

Sming/SmingCore/Network/HttpRequest.cpp

 	do
 	{
-		nextLine = NetUtils::pbufFindStr(buf, "\r\n", line);
+
+		nextLine = tmpbuf.indexOf("\r\n", line+2);


Shouldn't the last parenthesis be placed before the "+" operator?

I`ll verify it again

PTDreamer · 2016-03-02T11:57:38Z

No comments regarding coding style since I found no reference to the style adopted by this project. Linux Kernel style is the most common for this kind of projects and this PR doesn't conform in several places.
I don't fully understand the idea behind processing data chunks as they come instead of just increment (append data to) a buffer waiting for a complete header and process that. It would simplify code and increase readability (IMHO) without any performance impact.
PS: Don't know if my comments are welcomed or not since I haven't contributed with any code to this repo/project, but honestly I had nothing better to do :).

patrickjahns · 2016-03-02T12:11:53Z

@PTDreamer

See the Summary on the first page - there is the compatibility listed to arduino libraries ...
Any further discussion in regard to this should rather be in a different ISSUE - to keep things related to this PR

In regards to processing packets as they arrive (actually you can relate to this as a concept of processing a stream) - it is more memory friendly. While header data might be "small" (compared to post requests) - it requires more ram than just keeping unprocessed packet data in a buffer.

Before this PR there was no processing of headers split accross packets - (see #615 )
This is also meant as basis for processing multipart/post requests - a feature which I am working on in a different PR. While your idea of processing the header at once works for the header - it will not properly work for POST bodies - especially not for multipart requests - latest then I would need to implement the concept - and why not process both the same way? It is more intuitive than to process header and post requests differently

PTDreamer · 2016-03-02T15:43:33Z

See the Summary on the first page - there is the compatibility listed to arduino libraries ...
Any further discussion in regard to this should rather be in a different ISSUE - to keep things related to this PR

Not sure if the comment is related to the coding style or discussion about the substring function.

it is more memory friendly. While header data might be "small" (compared to post requests) - it requires more ram than just keeping unprocessed packet data in a buffer

It is not obvious to me how that is true. You are trading raw data buffer space with hash stored values which looking at the implementation bloats the amount of memory needed to store the data (which is true for all hash implementations).

Before this PR there was no processing of headers split accross packets - (see #615 )

No argument there. It is obvious there was a bug, the data will need to be appended to a buffer between callback calls or it will get ditched.

While your idea of processing the header at once works for the header - it will not properly work for POST bodies

Why not? You can just buffer the stream until you find "delimiter "--"" and parse it then.

In the end your code should work fine (although I still think you have a misplaced parenthesis), it is just a matter of personal coding style. Long do/whiles and lots of global variables are not my thing, also I found this PR hard to read not only by the code style but also by the variable naming.
Good job though, I hope I can find something to contribute with so you can take a turn at bashing my code.

patrickjahns · 2016-03-02T16:03:36Z

Not sure if the comment is related to the coding style or discussion about the substring function.

It was related to the discussion in regards to substring and compatibility with arduino libraries. And as suggested - i think this is best suited to be discussed in it`s own issue since it is not related to this PR

It is not obvious to me how that is true. You are trading raw data buffer space with hash stored values which looking at the implementation bloats the amount of memory needed to store the data (which is true for all hash implementations).

You need to differentiate between the buffer and the Hashmap - the Hashmap implementation was already in use even before I suggested the buffer. When keeping everything in one buffer before parsing, you would need the space for the buffer and the hashmap at the same time. Parsing as far as it goes, the buffer data needed can be smaller and leave room for the final hashmap to be worked with.

Why not? You can just buffer the stream until you find "delimiter "--"" and parse it then.

Since multipart requests are not only variables/fields to be kept in memory, but also can be files - I don`t see your suggestion viable - it would need again 2 different implementations for one task. For suggestions/discussion on multipart and file uploads please join #552

In the end your code should work fine (although I still think you have a misplaced parenthesis), it is just a matter of personal coding style. Long do/whiles and lots of global variables are not my thing, also I found this PR hard to read not only by the code style but also by the variable naming.

Please compare the version before and after my PR - I based my solution on the already exiting coding style. In what way would you improve the current implementation ?

Good job though, I hope I can find something to contribute with so you can take a turn at bashing my code.

I hope you dont see this discussion as a need for bashing code - as long as it improves code and usability in the end - its good to discuss it. And I am not too well suited for doing c/c++ code - I am more home with python. But since I am working on a project with an esp - I might as well help to improve it right? ;-)

hreintke · 2016-03-03T10:04:15Z

@patrickjahns :

In the end your code should work fine (although I still think you have a misplaced parenthesis)
Is there still an update needed from your code.?

On In the PR now there is

Merge pull request #1 from SmingHub/develop …
That contains (part of ?) another PR.
I am not the most experienced in git so am not sure how that effects the merging.
Any change that you can update the PR for that ?

patrickjahns · 2016-03-03T11:49:28Z

@hreintke

I had trouble squashing - will try again later and update accordingly. Please wait with merging until then. I still want to verify the part with the misplaced parenthesis - didn`t get around to it until now

hreintke · 2016-03-03T11:50:21Z

OK Clear

patrickjahns · 2016-03-04T02:12:29Z

@PTDreamer
Thanks for the hint with the line numbers - the +2 calculation was unnecessary and was a leftover from previous testing. Removed and now the PR is clean

@hreintke
squashed and ready to merge - I can prepare another PR for RTOS then

patrickjahns · 2016-03-12T13:50:54Z

@avr39-ripe
Mentioned that query parameters are not parsed/parsed correctly - will need to have another look

avr39-ripe · 2016-03-12T14:42:40Z

Nither queryParameter nor urlencoded form data DO NOT work ;(( switching to regular sming/develop solve the problem.. and introduce new one with packet hassle

patrickjahns · 2016-03-15T11:50:19Z

@avr39-ripe

pushed the proposed fix - please test and confirm that query and post parameters are working now as expected (please test only this branch and don`t include rawbody yet)

avr39-ripe · 2016-03-15T12:38:26Z

cherry-pick last two commits on top of develop and it works. tested by using response.getQueryParameter(). Thanks!

patrickjahns · 2016-03-15T12:55:10Z

I`d say this is ready to merge - maybe someone else can also confirm ?

avr39-ripe · 2016-03-15T14:12:18Z

If @dobrishinov try this and confirm.. He expect problems with split/truncated data.. but it needs rawBody fix also..

PTDreamer reviewed Mar 1, 2016
View reviewed changes

hreintke mentioned this pull request Mar 3, 2016

add check for length of header buffer (fixes #585) #613

Closed

buffer for parseHeader and parsePostData to handle split data

ed66036

patrickjahns force-pushed the http-buffer branch from 144e770 to ed66036 Compare March 4, 2016 02:07

patrickjahns mentioned this pull request Mar 4, 2016

Fix rawbody being empty #631

Merged

fix query/post param parsing

22e9267

avr39-ripe mentioned this pull request Mar 24, 2016

Rewrite Basic-Web_Skeleton_App to reflect comments on github/gitter #669

Closed

patrickjahns mentioned this pull request Mar 29, 2016

Bug: The header field name is case-insensitive but is not processed as such. #672

Closed

hreintke merged commit 22e9267 into SmingHub:develop Mar 31, 2016

patrickjahns mentioned this pull request Apr 4, 2016

Bug: missing header information (if header split between tcp packets) #615

Closed

patrickjahns deleted the http-buffer branch April 7, 2016 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating a buffer for parseHeader and parsePostData #622

Creating a buffer for parseHeader and parsePostData #622

patrickjahns commented Feb 27, 2016

patrickjahns commented Mar 1, 2016

PTDreamer Mar 1, 2016

patrickjahns Mar 1, 2016

PTDreamer commented Mar 2, 2016

patrickjahns commented Mar 2, 2016

PTDreamer commented Mar 2, 2016

patrickjahns commented Mar 2, 2016

hreintke commented Mar 3, 2016

patrickjahns commented Mar 3, 2016

hreintke commented Mar 3, 2016

patrickjahns commented Mar 4, 2016

patrickjahns commented Mar 12, 2016

avr39-ripe commented Mar 12, 2016

patrickjahns commented Mar 15, 2016

avr39-ripe commented Mar 15, 2016

patrickjahns commented Mar 15, 2016 via email

avr39-ripe commented Mar 15, 2016

Creating a buffer for parseHeader and parsePostData #622

Creating a buffer for parseHeader and parsePostData #622

Conversation

patrickjahns commented Feb 27, 2016

patrickjahns commented Mar 1, 2016

PTDreamer Mar 1, 2016

Choose a reason for hiding this comment

patrickjahns Mar 1, 2016

Choose a reason for hiding this comment

PTDreamer commented Mar 2, 2016

patrickjahns commented Mar 2, 2016

PTDreamer commented Mar 2, 2016

patrickjahns commented Mar 2, 2016

hreintke commented Mar 3, 2016

patrickjahns commented Mar 3, 2016

hreintke commented Mar 3, 2016

patrickjahns commented Mar 4, 2016

patrickjahns commented Mar 12, 2016

avr39-ripe commented Mar 12, 2016

patrickjahns commented Mar 15, 2016

avr39-ripe commented Mar 15, 2016

patrickjahns commented Mar 15, 2016 via email

avr39-ripe commented Mar 15, 2016