jdev - 2020-02-13

Pushed out a description of the changes I was asking about yesterday, in case anyone is curious or has feedback: https://github.com/mellium/xmpp/blob/master/design/0002_iqmux.md

WIP, but the important bits are laid out.

Sam Whited, does the API allow to register IQMux's that are not an IQ?

"func HandleIQ(iqType stanza.IQType, n xml.Name, h IQHandler)" does not look that way?

actually I am not sure if this is the function to register a handler or not

flow: yah, sorry, without more context this is likely confusing. The xmpp package has a HandleXMPP function used to register a single handler for all stanzas. You can choose to use this mux package (or write your own) to register a mux as that handler. The mux can have its own handlers.

hmm, no, does not look that way

Similarly, the IQHandler has to be registered on the mux for *all* IQs, and then it has its own handlers for IQs.

So the question would be what happens if the IQMux is registered as the handler for eg. messages instead of IQs?

Sam Whited, FWIW Smack registers IQ request handlers by QName and iq-request type (get|set), nothing more. IQ request handlers are an first-class citizen besides (what one could call) generic stanza handlers in Smack. Those are not able to handle IQ requests, which was a deliberate design choice so that smack is able to reply with an error if no IQ request handler is registered for a given IQ ✏

Yah, we don't have a concept of handlers for different things built in by default because the main library is supposed to be pretty low level. You just get a single handler that all top level stream elements (that aren't stream:stream elements) get passed to.

It also means the routing can be swapped out; if you don't like this simple router, you can create your own (it just has to implement the handler interface) that does whatever that XML query language is or whatever you prefer

Sam Whited, I assume this is not meant as pure xmpp client library? Guess things are different then

Then I guess it is by design that the API appears to allow you to install a handler for 'error' type IQs

That's a question I have too at the bottom; right now if you send an IQ (this is all part of a separate API) and you get an IQ result or error, that gets automatically passed back to the function you sent the IQ from so that the user can do something with it (see https://pkg.go.dev/mellium.im/xmpp#Session.SendIQ). However, if that has timed out and the function has returned, should it be passed to this handler for the user to handle or ignore? Eg. they may want to log it as a timeout or unhandled response.

I think the answer is "yes", because even if it's timed out I don't think anything should ever be sent over the stream that's automatically ignored, the user should have the option of doing something with it. But I'm not 100% sure.

And it is meant as a pure XMPP client library, if you mean "does it follow the spec?", but parts of it are pretty modular. If the users swap out the standard compliant parts to support some weird Cisco or Google brokenness, that's fine.

(but they'll have to do that, I don't develop them as part of the main project)

When we handle an incomming IQ too (regardless of whether a handler exists or not), we monitor what's being written back to the stream from the handler. If no error or result is written, when the handler returns the library automatically responds to the IQ with some generic result or error, I forget what.

Sorry for the wall of text to answer both questions.

I never had the desire to handle timed-out IQ responses (but smack would allow for that)

(or actually, on this layer it is currently undecidable in smack if someone is still waiting for the IQ response or not)

Don't get me wrong, it sure could be helpful to have that as diagnostic feature (e.g., "your timeout is to low"), it just isn't a high priority to implement for me

Yah, probably not the end of the world. Although, I think I convinced myself just now that the behavior should be that this can handle Error and Result types (with a note about how you'll only receive them if a timeout happened or the user specifically sent them with one of the raw token encoding APIs instead of SendIQ or SendIQElement which are the two things that actually handle responses)

So I may add two more options for that.

I also wonder if there's a more general way to handle this; eg. does it make sense to have a handler for messages and presence? I don't think so since I don't know what it would even match on, but I'm not sure

Without having to decode the entire stanza up front, that is.

I think one could do the decoding lazily

Actually, I was forgetting that error IQs will have this same problem. I always decode lazily, but if the error isn't the first child element, I don't want to match on the original payload and execute a handler for it.

but besides that, you have to parse the input xml anyway to determine the stanza boundaries, might as well decode it (at least a little bit). I think what smack does could be described as decoding the stanza always

This library deliberately doesn't do that; it operates on XML tokens instead of entire elements (then gives you the option to decode the tokens into a struct or map or however you do things in your handlers)

tbh, I don't see the advantage

Actually, this is fine, I'll just special case error and instead of matching on the child payload that always matches the whole IQ. I think that will be okay. It's a bit weird, I hate special cases, but error is a special case in the protocol so it makes sense that it would have to be in the library too.

No, I don't even have to special case it. This is up to the user, I'll just document it. If you're registering a handler for errors, better make it a wildcard on the XML name. Easy.

flow: the benefit in this case (which admittedly is niche and most people don't need it, but this is a lower level library meant to make an XMPP client library on top of) is constant memory use (more or less).

Sam Whited: At least at the library level. Pretty much every application is going to need to do a full decode in the end.

(Or I misunderstood the point)

Kev: yup, and now it's up to the user writing an XMPP client library with this backing it how to do that. They may have their own schemas for specific types, they may have a generic XML representation, etc.

Sam Whited, so entities inspecting a stanza only have a linear stream of xml events available?

is it really constant memory use or *minimal* memory use?

flow: yes, when you register a handler every time a new top level stream element is encountered it is given the start element that triggered the call (eg. a new <message>) and a thing that can be used to pop more tokens from the stream (but is limited to the element the handler is called on, it won't give you anymore if you try to advance beyond </message>). When the handler returns, the library advances to the end of that element (if there are any tokens left to pop) and then continues.

flow: it's not exactly constant, but we're generating less garbage than if we were converting those tokens into a struct or whatever. It is pretty minimal (or would be if the Go xml library weren't terrible, but that can be swapped out later)

And it lets the user convert those into whatever representation they want.

cause I'd say once Smack has consumed an xml stanza the resulting java object instance also has constant memory use ;)

The hypothesis is that tokens are all going to be around the same size (excluding character data payloads sometimes, eg. messages or IBB data, etc.). But if you were decoding things you'd have to both have those tokens *and* decode them into a struct or whatever your data type is.

That's the hypothesis anyways: tokens are probably all roughly similar size and you can throw them away quicker instead of having to buffer all of them and do parsing and create more objects. That's up to the user to decide how they do that in this case.

oops, somehow I thought that errored the first time, sorry for the rephrase of the same thing.

But the memory use is really less interesting than just being low level and letting the user decode data however they want I think.

After experimenting with it, I'm very happy to just have a shortcut to register a handler for all Error type IQs that ignores the payload. Doesn't feel special cased, makes it easy to log errors that weren't handled for one reason or another or do manual error handling if you don't like the way the library waits for an error response

Pushed an update with the new API and updated comments to explain it

Thanks for all the feedback, just talking through it out loud has been really helpful too.

Always happy to act as rubber duck ;)

Hmm, rubber ducks as swag for FOSDEM and such?

🦆️<(use XMPP)