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

Fifo fixes #3638

merged 3 commits into from Feb 25, 2024

Conversation

HHHartmann
Copy link
Member

@HHHartmann HHHartmann commented Feb 18, 2024

Fixes problems I had during development.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

  • This PR is for the dev branch rather than for the release branch.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/*.

To be completed below: Description of and rationale behind this PR.

This contains fixes for problems I found while using fifo.

@HHHartmann HHHartmann mentioned this pull request Feb 18, 2024
Copy link
Member

@nwf nwf left a comment

Choose a reason for hiding this comment

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

LGTM. Some commentary just to make sure I understand the changes.

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.

@@ -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.

@marcelstoer
Copy link
Member

@HHHartmann as it's been approved, do you want to merge this? Or do you still plan to add changes given @nwf comments?

@HHHartmann
Copy link
Member Author

Nope, not planning to add changes.

@marcelstoer marcelstoer merged commit 3d09b74 into nodemcu:dev Feb 25, 2024
18 checks passed
marcelstoer pushed a commit that referenced this pull request Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants