Added request body buffer regeneration by dylandreimerink · Pull Request #43 · openresty/lua-resty-upload

dylandreimerink · 2018-03-29T14:28:58Z

I made a fix for issue #41 and #42.
Optional support for body buffer regeneration.

bungle · 2018-04-02T09:02:02Z

lib/resty/upload.lua

+end
+
+local function finish_body(self)
+    if self.resore_body then


bungle · 2018-04-02T09:03:57Z

lib/resty/upload.lua

        sock = sock,
        size = chunk_size or CHUNK_SIZE,
        line_size = max_line_size or MAX_LINE_SIZE,
+        resore_body = restore_body_buffer or false,


resore -> restore? Also or false not needed, though, doesn't hurt much either.

bungle · 2018-04-02T09:06:04Z

lib/resty/upload.lua


        local data = sock:receive(2)
        if data == "--" then
+            append_body(self, "\r\n--" .. self.boundary .. "--\r\n")


We always concatenate strings here. Even when restore_body is false. Same thing happens in other append_body places. Maybe just checking the boolean variable doesn't warrant its own function and that can be inlined. Then we also solve this issue.

bungle · 2018-04-02T09:10:05Z

lib/resty/upload.lua


    local line, err = read_line(self.line_size)
    if not line then
+        finish_body(self);


semicon ; not needed.

dylandreimerink · 2018-04-02T13:21:15Z

I fixed the typo and implemented you suggestion. I am not sure though if it is faster to append multiple times like I have done now or to create a new string and append once.

bungle · 2018-04-02T17:03:38Z

Yes, previously you created strings even when not appending anything.

bungle · 2018-04-02T17:06:28Z

lib/resty/upload.lua

    return sock:settimeout(timeout)
 end

+local function append_body(self, ...)


I don't think you need to use varargs here, as you only ever have at most 3 args. Also you don't need to create a table with varargs, you can select("#", ...) and select(1, ...).

bungle · 2018-04-02T17:07:24Z

@dylandreimerink thanks for working on this. I think that one thing to add is test(s).

dylandreimerink · 2018-04-03T14:37:55Z

I took a look at the source code for ngx.req.append_body which is quite big and does a memcpy. So I think it is best to ditch the whole idea of a separate function ( like u suggested before ) and concatenate a single string in the main functions. Then calling ngx.req.append_body once right before ngx.req.finish_body to minimize overhead.

I will work on that as soon as I have time

dylandreimerink · 2018-04-04T20:25:45Z

This should do it. I replaced the appending by a string buffer which should make it more performant. If this passes the only thing left to do is to make tests. I have to admit however that I do not yet understand how this unit test framework works, so i may need a little bit of guidance.

bungle · 2018-04-04T20:57:32Z

I am not sure what does it add to buffer to Lua table? Just more complexity. For real string buffer we need something like this: LuaJIT/LuaJIT#14

I think that just ngx.req.append_body inline without separate function is the best still. Even append that \r\n without ... Lets just not be to clever here. The function is called append, and that should be fast already. If that turns to be issue, we need to prove it first, before adding table buffers etc.

dylandreimerink · 2018-04-04T22:58:53Z

Working from the assumption that append_body is relatively speaking slow since it has to copy data from the lua vm to nginx the idea is to keep the data in lua for as long as possible. Since lua strings are immutable they have to be recreated and copied every time you want to concatenate to an existing string. The solution would be to have a native implementation of a mutable string or string buffer. But since I have little hope for that in the near future I think the use of a table to store a list of strings and concatenating them once is the best option.

The reason I am going to these lengths is because I will be using this feature in a proxy with a high throughput, so for me personally speed and optimization is quite important. So in light of that I will benchmark the different solutions so we can make an informed decision instead of having to work based on assumptions.

dylandreimerink · 2018-04-07T14:31:08Z

I made the benchmark. The results are to me a bit unexpected. I tested 4 variations and the original, the summarized results are as follows:

Name	total time	time per req (avg)	reqests per second(avg)
Base test	87.77	8.777	11393.38
complex string buffer test	90.987	9.099	10990.54
simple string buffer test	90.971	9.097	10992.56
string concat test	90.843	9.084	11008.04
No buffer test	90.233	9.023	11082.48

The variation without the string buffer is the fastest but the difference is almost negligible.
So I went with that solution.

dylandreimerink · 2018-04-12T10:31:56Z

@bungle Have you had time to look at the last changes?

bungle

I left some more comments. Mostly style related changes. Biggest pressing issue right now is that this doesn't yet have tests. I think we should have good test suite that also tests with invalid input. Should we actually have some kind of internal error handler here that runs in case of error happens, and then just writes rest of the buffer back, e.g. leaving the body intact.

bungle · 2018-04-17T14:33:13Z

lib/resty/upload.lua

    if not read_line then
+
+        if restore_body_buffer then
+            ngx_finish_body()


The previous read, on line 72, was successful, but we didn't write the request body back, is this intended? Maybe there are many similar cases in code, but no easy way to solve? E.g. on errors we don't write the request body back.

The sock:receiveuntil returns an iterator Lua function and doesn't actually read data from the buffer so there is noting to restore.

bungle · 2018-04-17T14:34:40Z

lib/resty/upload.lua

    end

+    if self.restore_body_buffer then
+        ngx_append_body(line .. "\r\n")


I think this is fine, but you could also have:

ngx_append_body(line) ngx_append_body("\r\n")

I don't see any point in not concatenating the string here, the same goes for the rest of the remarks regarding concatenation.

bungle · 2018-04-17T14:40:13Z

lib/resty/upload.lua

        local data = sock:receive(2)
        if data == "--" then
+            if self.restore_body_buffer then
+                ngx_append_body("\r\n--" .. self.boundary .. "--\r\n")


Same here:

ngx_append_body("\r\n--") ngx_append_body(self.boundary) ngx_append_body(ngx_append_body("--\r\n"))

Not a big deal.

bungle · 2018-04-17T14:41:21Z

lib/resty/upload.lua

        end

+        if self.restore_body_buffer then
+            ngx_append_body("\r\n--" .. self.boundary .. "\r\n")


This coud be cached before the while loop:

local boundary_delimiter = "\r\n--" .. self.boundary .. "\r\n"

And then here:

ngx_append_body(boundary_delimiter)

There is no while loop surrounding this line

bungle · 2018-04-17T14:43:56Z

lib/resty/upload.lua

        local preamble = read2boundary(size)
        if not preamble then
+            if self.restore_body_buffer then
+                ngx_append_body("--" .. self.boundary)


Ditto here:

ngx_append_body("--") ngx_append_body(self.boundary)

Yes, bikeshedding :-).

bungle · 2018-04-17T14:53:51Z

lib/resty/upload.lua

    if dummy then
+        if self.restore_body_buffer then
+            ngx_finish_body()
+        end


new line after this.

bungle · 2018-04-17T14:54:00Z

lib/resty/upload.lua

    if err then
+        if self.restore_body_buffer then
+            ngx_finish_body()
+        end


new line after this.

bungle · 2018-04-17T14:54:09Z

lib/resty/upload.lua

        if err and err ~= 'closed' then
+            if self.restore_body_buffer then
+                ngx_finish_body()
+            end


new line after this.

bungle · 2018-04-17T14:54:17Z

lib/resty/upload.lua

    if err then
+        if self.restore_body_buffer then
+            ngx_finish_body()
+        end


new line after this.

bungle · 2018-04-17T14:54:32Z

lib/resty/upload.lua

    if err then
+        if self.restore_body_buffer then
+            ngx_finish_body()
+        end


new line after this.

dylandreimerink added 2 commits March 29, 2018 12:47

Added request body buffer regeneration

74bfc12

Fixed missing \r\n's and first boundary

4ce90bb

bungle requested changes Apr 2, 2018

View reviewed changes

Fixed typo and removed unnecessary concatenation

ce8ddd8

bungle reviewed Apr 2, 2018

View reviewed changes

Replaced multiple body append by a string buffer and single body append

f9c3a30

dylandreimerink added 2 commits April 7, 2018 16:22

Cleaned up unnecessary code

ef5cb46

Removed body_buffer variable

458fb3a

bungle reviewed Apr 17, 2018

View reviewed changes

dylandreimerink added 2 commits April 28, 2018 20:13

Made code standards changes

79836a0

Changed elseif to else if

873f235

dvershinin mentioned this pull request Sep 5, 2019

POST data is lost bungle/lua-resty-reqargs#11

Open

suikabreaker mentioned this pull request Jan 12, 2022

feature: preserve body after parse #63

Merged

dylandreimerink closed this Jun 17, 2022

Comments

Conversation

dylandreimerink commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylandreimerink commented Apr 2, 2018

Uh oh!

bungle commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bungle commented Apr 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylandreimerink commented Apr 3, 2018

Uh oh!

dylandreimerink commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bungle commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylandreimerink commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dylandreimerink commented Apr 7, 2018

Uh oh!

dylandreimerink commented Apr 12, 2018

Uh oh!

bungle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bungle commented Apr 2, 2018 •

edited

Loading

dylandreimerink commented Apr 4, 2018 •

edited

Loading

bungle commented Apr 4, 2018 •

edited

Loading

dylandreimerink commented Apr 4, 2018 •

edited

Loading

bungle left a comment •

edited

Loading