AsyncClient::subscribe_many_with_options loses options if there is only one filter

[qm3ster]

Due to the following code in the C library, which IMHO is questionable:

sub->command.properties = MQTTProperties_copy(&response->properties);
sub->command.details.sub.opts = response->subscribeOptions;
if (count > 1)
{
	if ((sub->command.details.sub.optlist = malloc(sizeof(MQTTSubscribe_options) * count)) == NULL)
	{
		rc = PAHO_MEMORY_ERROR;
		goto exit;
	}
	if (response->subscribeOptionsCount == 0)
	{
		MQTTSubscribe_options initialized = MQTTSubscribe_options_initializer;
		for (i = 0; i < count; ++i)
			sub->command.details.sub.optlist[i] = initialized;
	}
	else
	{
		for (i = 0; i < count; ++i)
			sub->command.details.sub.optlist[i] = response->subscribeOptionsList[i];
	}
}

(found at https://github.com/eclipse/paho.mqtt.c/blob/64a5ff3c3b71fe019353aeacaebc66a3cf4f3461/src/MQTTAsync.c#L1029-L1048)
when passed in count is count is 1 (or 0 lol?) only opts is written in, not optlist.

Since only ResponseOptionsBuilder::subscribe_many_options and not ResponseOptionsBuilder::subscribe_options is called in AsyncClient::subscribe_many_with_options, opts remains at the default value, and that's what makes it into the packet.
While I think this should be fixed in the C library, by differentiating into more concrete functions (and structs for their arguments), we can guard against it by additionally setting opts to the only option byte when the count is 1.
(We should still set optlist in case this behavior improves, and a change in the C library API would require/suggest modifying these functions anyway, which is when this hacky accommodation can be removed)

[fpagliughi]

Oh, damn, sorry. I fixed this in the C++ library a looooong time ago, but keep forgetting to fix it here as well.

If I recall, the attitude for the C lib was to avoid the extra malloc when there's only one response. So this is considered a feature/optimization on their side; not a problem. So it won't be "fixed" there. I'll have a look at the PR shortly.

[fpagliughi]

Oooh... wait. This is not the issue I was thinking of! (I was remembering the problem in the callback when there's just a single response... there's no array of values and I was segfaulting on the NULL pointer). OK. I see it now.

[qm3ster]

Yeah, the other one was in #51 :v

[qm3ster]

So, you're saying that API is set in stone?
Should we also skip the subscribe_many_options call then, and avoid allocating the Vec?

[fpagliughi]

Well the choice is left up to the app. If it knows that wants to subscribe to one topic, then the single subscribe is slightly more efficient, but I always wonder about overly optimizing something like this when (1) most apps don't do a lot of subscribing and unsubscribing, and (2) the round-trip network call is gonna take hundreds of times longer than the malloc anyway.

[qm3ster]

I don't understand so many things about that design.

1. Why put it in MQTTAsync_responseOptions where it will go unused for all async calls except those that produce subscriptions? Just add it to the argument list of MQTTAsync_subscribeMany which is where it belongs?
2. Why have all 3 of

MQTTSubscribe_options subscribeOptions;
int subscribeOptionsCount;
MQTTSubscribe_options* subscribeOptionsList;

? They were doing spectacularly well, thriving even, with qos, by exploiting the fact they can pass a pointer higher into the stack as array, and here we couldn't disambiguate by subscribeOptionsCount:

• 0 => use defaults for each filter
• 1 => this is a value
• 2 => this is a pointer to array
  (or just don't do that microoptimization that adds branching, and dereference pointer for count = 1 as well)

3. there are still unconditional

sub->command.details.sub.topics = malloc(sizeof(char*) * count);
sub->command.details.sub.qoss = malloc(sizeof(int) * count);

if we assume the mallocs are optimized together/away, it would be better to put the optlist malloc next to them unconditionally.
If we don't rely on such assumptions, it would probably better to allocate one sausage of (char*, int, MQTTSubscribe_options) structs?

4. and then MQTTStrdup allocates count times in a loop. Surely if we fear mallocs it would be better to strlen all the strings, (maybe storing their lengths in a preallocated size_t * count for the second step, or maybe not), then make one allocation for all the string contents?

I understand subscription calls are not a bottleneck and never will, but that optimization that impacts the API (and hotter calls like publish, with slightly larger MQTTAsync_responseOptions!) seems at odds with that.

Disclaimer: I don't understand C or performance.

[fpagliughi]

Yeah, if you look through issues/emails going back through the history of the C lib, you'll see me questioning and arguing about design decisions. Some is legacy (remembering that the library is probably like 15yo).

But keep in mind... for the last 8ys that I've been tracking it, they have continued to to add to the library a crazy number of features, including full MQTT v5 support... without breaking the API once.

That's pretty impressive. But I do keep thinking that it's time for a v2 that cleans up a lot of the weirdness. It's just that no one has the time or funding to do it.