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

Update node version and transitive deps #426

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

xylo04
Copy link
Contributor

@xylo04 xylo04 commented Oct 16, 2023

This updates Node and NPM to the latest Long Term Supported versions. Node is imminently upgrading v20 to LTS, so we should at least get this far.

@martinhpedersen martinhpedersen added the dependencies Pull requests that update a dependency file label Oct 16, 2023
@martinhpedersen
Copy link
Member

Thanks :D

While I was testing this I saw a very strange bug. One of the messages I received was truncated before I got to reading it. Either that, or the file was written as empty (0 bytes) without causing any errors. The output view (web) refreshed after CMS exchange, and then it errored out on io.EOF when trying to read the message.

I was testing on my Android phone, so I have a feeling this is not related to the changes in the PR. Nevertheless, having a message truncated after being ACKed (and thus deleted from CMS) is a bug worth investigating. The message is gone. I've read through the Go and JS code multiple times now, and can't understand how this could have happened.

The only thing I can think of related to this PR is the setRead JS function which triggers a re-write of the message on disk. Might be a race related to that, but I can't understand why/how it was triggered if that's the case.

@martinhpedersen
Copy link
Member

So I did some more digging, and found a case where we would end up writing an empty .b2f file that could explain the bug I observed.

If the B2F message we receive is a headerless message (or a message starting with the header delimiter), we end up not reporting any error and just writing an empty message to disk. The empty file written to disk as a result will produce the error I observed when testing this PR. Unfortunately, since the buggy message was lost I can't confirm my theory.

I think it's safe to say the bug isn't related to this PR. I'll merge this asap, but first some sleep 🛌 🙂

@@ -1,6 +1,6 @@
{
"name": "pat",
"version": "0.12.1",
"version": "0.15.0",
Copy link
Member

Choose a reason for hiding this comment

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

Side note: Maybe we should set this to 0.0.0 since it's not used anywhere and we keep forgetting to update it.

@martinhpedersen martinhpedersen merged commit 4f1b7e5 into la5nta:develop Oct 18, 2023
4 checks passed
@martinhpedersen martinhpedersen added this to the v0.15.1 milestone Oct 18, 2023
@xylo04 xylo04 deleted the update-web branch August 12, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants