Immutable connection and limiting outgoing messages on buffer

[najaco]

Currently in netlink-proto's ConnectionHandle the functions request and notify require that the caller be mut, despite there being no calls that require it's only member (the UnboundedSender) to be &mut.
By forcing mutability, it prevents clients from being able to pass the connection handle into futures, which would be ideal for scheduling asynchronously.

After fixing this, I noticed the following issue: after scheduling a significant number of requests, Connection::poll_send_messages would attempt to write as many messages as possible to the buffer before being sent to the netlink socket. When the whole buffer was sent, it appeared that, on the kernel side, the netlink socket would try to respond to every message on the buffer at once, and would overflow its own returning buffer.

Since there was not a deterministic way of knowing how much the kernel would write to the outgoing buffer, it was safest to swap out the ability to send multiple netlink requests with the limitation to only send one at a time. With this, we can safely pass the connection handle to a significant number of futures without having to worry about the kernel returning an OS error.

[najaco]

@cathay4t Do you have any thoughts about this?

[najaco]

@koalo Could you please take a look?

[koalo]

I do not quite understand if you mean the second issue is triggered by fixing the first one or if this is two completely independent issues (in which case please split it up into two pull requests).

[najaco]

@koalo Could you please take another look?

[najaco]

@cathay4t any thoughts?

[najaco]

@koalo Any update on this?

[zooknotic]

I have reproduced the same issue with Connection::poll_send_messages(), and pretty much came up with the same solution independently before seeing this report. If the socket is Pending, it will keep accumulating a larger and larger message buffer, and when the socket becomes Ready again, it will attempt to send one gigantic message to the kernel. In addition to the reporter's description of the response message being too large, the send to kernel message can also be too big. Below is strace output of the sendto() message to the kernel failing with -1 EMSGSIZE (Message too long). Incidentally, this error isn't propagated back to the app.

[pid 115877] write(1, "2023-08-13T02:46:45.721Z TRACE ["..., 772023-08-13T02:46:45.721Z TRACE [netlink_proto::connection] poll_flush called ) = 77 [pid 115877] write(1, "2023-08-13T02:46:45.721Z TRACE ["..., 852023-08-13T02:46:45.721Z TRACE [netlink_proto::framed] flushing frame; length=431840 ) = 85 [pid 115877] sendto(6, [[{nlmsg_len=32, nlmsg_type=RTM_DELLINK, nlmsg_flags=NLM_F_REQUEST, nlmsg_seq=632, nlmsg_pid=0}, {ifi_family=AF_UNSPEC, ifi_type=ARPHRD_NETROM, ....) = -1 EMSGSIZE (Message too long) [pid 115877] write(1, "2023-08-13T02:46:45.722Z WARN ["..., 1562023-08-13T02:46:45.722Z WARN [netlink_proto::connection] error flushing netlink socket: Os { code: 90, kind: Uncategorized, message: "Message too long" } ) = 156 [pid 115877] write(1, "2023-08-13T02:46:45.722Z TRACE ["..., 832023-08-13T02:46:45.722Z TRACE [netlink_proto::connection] done polling Connection ) = 83

[koalo]

@najaco I don't know why you keep polling me since I am not a maintainer of this project and I am still of the opinion that should should be split up into two pull requests or at least two commits (an 'and' in the title is very often an indication to split up the commit...).

[najaco]

@koalo I kept polling you because you were the only one responding originally. I am not getting any responses from the original maintainers so I am not exactly sure who I am supposed to ask, but I apologize for pestering.
Unfortunately this can't be two separate PRs, because the limiting of outgoing messages on the buffer is only necessary when the connection is immutable and passed to multiple futures.
We could limit it while it is mutable if you really wanted two separate PRs, but that is rather pointless because it isn't a problem when self is &mut since it cannot be passed to multiple futures anyways.

[koalo]

Just split it up into two commits in the same pull request, remove the unnecessary merge commit and then it should be clean to merge from my point of view (even if I am not able to do so...).

[najaco]

@koalo Done

[koalo]

Great, you can have my
Reviewed-by: Florian Kauer <florian.kauer@linutronix.de>
if that helps you in any way...

[cathay4t]

I have append new patch to fix the test failures.

Since this is big fundamental changes, please allow me to test this patch in rtnetlink for 4 weeks before tagging new release.

[cathay4t]

I have manually tested this patch in my projects, works well.

This patch will be contains in new 0.11.3 release: #21