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.

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

Fifo fixes #3638

Merged
merged 3 commits into from
Feb 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 19 additions & 15 deletions lua_modules/fifo/fifo.lua
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
-- A generic fifo module. See docs/lua-modules/fifo.md for use examples.

local tr, ti = table.remove, table.insert
local tableRemove, tableInsert = table.remove, table.insert

-- Remove an element and pass it to k, together with a boolean indicating that
-- this is the last element in the queue; if that returns a value, leave that
Expand All @@ -17,29 +17,33 @@ local tr, ti = table.remove, table.insert
--
-- Returns 'true' if the queue contained at least one non-phantom entry,
-- 'false' otherwise.
local function dequeue(q,k)
if #q > 0
then
local new, again = k(q[1], #q == 1)
if new == nil
then tr(q,1)
if again then return dequeue(q, k) end -- note tail call
else q[1] = new
end
return true
else q._go = true ; return false
local function dequeue(q, k)
if #q > 0 then
local new, again = k(q[1], #q == 1)
if new == nil then
tableRemove(q, 1)
else
q[1] = new
end
if again then
return dequeue(q, k)
end -- note tail call
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'd never had reason to both replace the top element of the fifo and want to be called back on the thing I just pushed. That does seem a little odd to want, but I agree that this patch is probably a better thing to do if the konsumer asks for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did these changes around two years ago, but the code runs better with it.
IIRC it was that I forced code to send a whole file into the httpserver.lua module including giving the size non chunked but at the beginning and so had some uh, well "interesting" usings of the whole thing.
Should have changed the api instead of hiding it behind specific parametrs, but well.
It just stopped sending in certain cases and I tracked it down to having to return what I just pushed.

return true
else
q._go = true
return false
end
end

-- Queue a on queue q and dequeue with `k` if the fifo had previously emptied.
local function queue(q,a,k)
ti(q,a)
tableInsert(q,a)
if k ~= nil and q._go then q._go = false; dequeue(q, k) end
end

-- return a table containing just the FIFO constructor
return {
['new'] = function()
return { ['_go'] = true ; ['queue'] = queue ; ['dequeue'] = dequeue }
end
return { ['_go'] = true ; ['queue'] = queue ; ['dequeue'] = dequeue }
end
}
10 changes: 9 additions & 1 deletion lua_modules/fifo/fifosock.lua
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,15 @@ local function wrap(sock)
-- Either that was a function or we've hit our coalescing limit or
-- we didn't ship above. Ship now, if there's something to ship.
if s ~= nil then
if sslan == 0 then sock:send(s) else sock:send(ssla .. s) end
if sslan == 0 then
if #s > 0 then
Copy link
Member

Choose a reason for hiding this comment

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

Sure, avoiding send-ing 0-length things seems like a good idea. And while return-ing ns or nil, true below means we won't call gc() this time, we will probably do so "soon" since we're about to be called back by the fifo logic.

sock:send(s)
else
return ns or nil, true
end
else
sock:send(ssla .. s)
end
ssla, sslan = nil, 0; gc()
return ns or nil, false
elseif sslan ~= 0 then
Expand Down
Loading