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

Set LINELEN to 8k for clients that support long messages #65

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

ToxicFrog
Copy link
Contributor

This matches the value already negotiated for cap multiline. I think we could go up to 16k without difficulty, but 8k is probably sufficient.

This also tells gen_tcp to use an 8k buffer; the default buffer size is about 1.5k (one TCP packet, maybe?) and cuts off longer commands, with tragic results.

@progval
Copy link
Owner

progval commented Feb 12, 2024

You can reuse the @multiline_max_bytes constant

cuts off longer commands, with tragic results.

this sounds like a different bug that would also be triggered if clients sent a single command split into two packets (eg. because it sent previous commands in the same packet)

This matches the value already negotiated for cap multiline. I think we
could go up to 16k without difficulty, but 8k is probably sufficient.

This also tells gen_tcp to use a 16k buffer; with the default settings,
long commands sometimes get split across multiple reads, with
unfortunate results.
@ToxicFrog
Copy link
Contributor Author

ToxicFrog commented Feb 12, 2024

You can reuse the @multiline_max_bytes constant

Done, thanks for the pointer.

this sounds like a different bug that would also be triggered if clients sent a single command split into two packets (eg. because it sent previous commands in the same packet)

It's not a packet-splitting issue, I was just speculating as to why that's the default buffer size, since it's suspiciously close to the ethernet MTU of 1500.

It looks like erlang IP networking sets the default as 1460b, and will not expand it automatically. Thus, if you ask for line-buffered mode without specifying a bufsize, and get a longer line than that, you will get 1460-byte line fragments followed by whatever's at the tail of the line. Which explains the behaviour I was seeing -- with just the change to LINELEN, weechat would cheerfully send long commands and they'd show up in the IRC protocol handler as multiple lines of ≤ 1.5k each.

In the commit as revised I set this to twice the LINELEN, just in case of misbehaving clients that (say) forget to take into account the command overhead when measuring out long commands, and base it too on @multiline_max_bytes.

@progval
Copy link
Owner

progval commented Feb 12, 2024

Ah right, that makes sense

@progval progval merged commit bac0f5b into progval:main Feb 12, 2024
8 checks passed
@ToxicFrog ToxicFrog deleted the linelen branch February 12, 2024 22:26
@ToxicFrog
Copy link
Contributor Author

Sorry, I messed up -- there's a line missing from handler.ex. Fix is here.

Perplexingly, even without that line mix test passes all test cases, although if you scroll up enough there is a stack trace from it failing to start the TCP server; this appears to be considered purely informational. I've never used mix or elixir before, so I don't know if this is expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants