Skip to content

Latest commit

 

History

History
79 lines (46 loc) · 7.75 KB

FORK.md

File metadata and controls

79 lines (46 loc) · 7.75 KB

Why this fork exists

This is mochi-mqtt/server, the MQTT library mast embeds to terminate MQTT. It is MIT licensed and all credit for it belongs upstream: mochi-co and the contributors listed there wrote it, and the SPDX headers on every file still say so.

It is detached from the GitHub fork network rather than being a fork in the GitHub sense, so it is a repository of its own with its own issues and its own module path.

The fork exists because upstream has stopped merging. The last commit to mochi-mqtt/server@main is dated 2025-03-01, which is also the date of the latest release, v2.7.9. There are 47 open pull requests, and #515, "Is the project still maintained?" has been open since August 2026 with no maintainer reply.

We depend on this library, so we carry the patches we need here rather than waiting.

What this fork carries

Change Upstream
Clients.GetByListener no longer takes the read lock recursively #488, fixed identically in #489
A peer that hangs up before sending a packet logs at debug, not warn, and the record has a message not reported upstream
The accept loops no longer assign to a shared err from inside their goroutines not reported upstream
A client that sets Request Problem Information to 0 still receives the user properties of a PUBLISH not reported upstream
PUBACK, PUBREC, PUBREL and PUBCOMP written while the next inbound packet is already buffered go out in one write #552, carried as is

Nothing else. Every other line is upstream's, and the intent is to keep it that way: a patch here should be one that upstream has already been offered and has not taken.

The deadlock

GetByListener held the read lock and then called Len, which takes the read lock again. sync.RWMutex documents that a reader arriving after a blocked writer waits for that writer, so a Lock landing between the two acquisitions wedges all three permanently — the second RLock waits for the writer, the writer waits for the first RLock to be released.

In practice the trigger is a client connecting while the server is shutting down: Server.Close reaches GetByListener through closeListenerClients, and attachClient reaches Clients.Delete for a client id that is already known. We found it as an intermittently hanging test suite, but the production shape is worse — a pod told to terminate never exits, and is eventually SIGKILLed with every connection dropped uncleanly.

clients_deadlock_test.go drives the race directly. Against unpatched code it wedges and fails on its 20-second deadline; patched it finishes in well under a second.

The health-check warnings

listeners/tcp.go, net.go and unixsock.go each ran l.log.Warn("", "error", err) for any failure to establish a connection. Two things were wrong with that.

The message was empty, so the record reached production as {"msg":"","level":"WARN","error":"read connection: EOF"} — a warning that says nothing.

And an io.EOF there is not a failure. It is a peer that opened a connection and closed it without speaking MQTT, which is what a TCP health check does. A Kubernetes readinessProbe with tcpSocket on the MQTT port and the default ten-second period produces 8,640 of these a day, per pod, which is how a logger stops being read. Found exactly that way, in a running deployment.

An ordinary hang-up now logs at debug; a failure after the client has started speaking MQTT still warns, because then something really did go wrong.

The same three lines also had a data race: err = establish(...) inside the spawned goroutine assigns to the err declared by the accept loop, and the loop keeps accepting, so several goroutines write the same variable concurrently.

User properties withheld from PUBLISH

A client may set Request Problem Information to 0 in CONNECT, and WritePacket turns that into Mods.DisallowProblemInfo, which Properties.Encode applied to user properties on every packet type. The spec exempts PUBLISH, CONNACK and DISCONNECT [MQTT-3.1.2-29], and for a PUBLISH it is not optional: user properties are the publisher's data, and the server must forward them unaltered [MQTT-3.3.2-17]. A subscriber that opted out of diagnostics silently lost the application's metadata on every message.

paho.golang sends 0 whenever a client passes connect properties without setting the flag, which is how mast's own tests found it. Only PUBLISH is exempted here; withholding them from CONNACK and DISCONNECT is still allowed, so that behaviour is left as upstream wrote it.

Coalesced acks

Every acknowledgement the read loop sends — PUBACK and PUBREC for an inbound PUBLISH, PUBREL for a PUBREC, PUBCOMP for a PUBREL — went through WritePacket on its own, and WritePacket buffered only while WriteLoop's outbound queue was non-empty. The read loop never queues there, so every ack was a write(2) of its own even when the reader already held the next twenty packets. Upstream #552 holds acks in outbuf while the next packet is complete in the read buffer, and flushes before the reader would block, after 1ms, when the buffer fills, on any other write, and on DISCONNECT.

It is carried exactly as offered, with only the import paths renamed. mast found the cost under QoS 2 load (mastmq/mast#18): a CPU profile of a node delivering 100k QoS 2 messages a second spent 90% of its time in socket syscalls, 37% of it under processPubrec. With the patch, 50 publishers at 100k QoS 2 deliveries a second went from a p50 of 3.3–4.0s, with the publishers falling behind, to 99–191ms with everything delivered.

What it cannot do is batch a connection with nothing behind its ack. A subscriber receiving a message every 10ms answers each one alone, so a QoS 2 delivery still costs the broker two writes and two reads, about 1.6× the CPU of a QoS 1 delivery. That is the protocol, not this code.

The module path

The module is github.com/mastmq/mochi/v2, not github.com/mochi-mqtt/server/v2. A detached repository has to declare the path it is actually served from, or go get fails on the mismatch.

That rename is mechanical — go.mod plus the import line in 50 files — and it is the only change here that is not upstream's code. It does mean a patch sent back upstream needs the rename stripped first. Given upstream has not merged anything since March 2025 that is a theoretical cost, but it is the reason to keep every other line identical.

Versions track upstream's and add a patch digit, so v2.7.10 is upstream v2.7.9 plus what is listed above. If upstream ever releases v2.7.10 of its own the numbers will collide in meaning but not in fact, because the module paths differ.

Working on it

upstream is configured for fetch only, so a stray push cannot reach mochi-mqtt/server:

$ git remote -v
origin    git@github.com:mastmq/mochi.git (fetch)
origin    git@github.com:mastmq/mochi.git (push)
upstream  git@github.com:mochi-mqtt/server.git (fetch)
upstream  no-push-use-origin (push)

Keep main as close to upstream as it can be. If upstream ever revives, the difference should be small enough to send back as a pull request and then drop from here.

Two things were removed rather than carried, because they belong to upstream and not to us: the docker job in .github/workflows/build.yml, which pushed to the mochimqtt/server Docker Hub repository with credentials we do not have, and the Coveralls and Stargazers badges, which would have reported another project's numbers as though they were this one's.