Sam WhitedPushed 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
Sam WhitedWIP, but the important bits are laid out.
flowSam Whited, does the API allow to register IQMux's that are not an IQ?
flow"func HandleIQ(iqType stanza.IQType, n xml.Name, h IQHandler)" does not look that way?
flowactually I am not sure if this is the function to register a handler or not
Sam Whitedflow: 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.
flowhmm, no, does not look that way
Sam WhitedSimilarly, the IQHandler has to be registered on the mux for *all* IQs, and then it has its own handlers for IQs.
Sam WhitedSo the question would be what happens if the IQMux is registered as the handler for eg. messages instead of IQs?
flowSam Whited, FWIW Smack registers IQ request handlers by QName and iq-request type (get|set), nothing more. IQ request handlers are an first-class concept 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
flowSam 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
Sam WhitedYah, 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.
Sam WhitedIt 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
flowSam Whited, I assume this is not meant as pure xmpp client library? Guess things are different then
flowThen I guess it is by design that the API appears to allow you to install a handler for 'error' type IQs
Sam WhitedThat'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.
Sam WhitedI 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.
Sam WhitedAnd 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.
Sam Whited(but they'll have to do that, I don't develop them as part of the main project)
Sam WhitedWhen 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.
Sam WhitedSorry for the wall of text to answer both questions.
flowI never had the desire to handle timed-out IQ responses (but smack would allow for that)
flow(or actually, on this layer it is currently undecidable in smack if someone is still waiting for the IQ response or not)
flowDon'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
Sam WhitedYah, 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)
Sam WhitedSo I may add two more options for that.
Sam WhitedI 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
Sam WhitedWithout having to decode the entire stanza up front, that is.
flowI think one could do the decoding lazily
Sam WhitedActually, 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.
flowbut 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
Sam WhitedThis 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)
flowtbh, I don't see the advantage
Sam WhitedActually, 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.
Sam WhitedNo, 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.
Sam Whitedflow: 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).
KevSam Whited: At least at the library level. Pretty much every application is going to need to do a full decode in the end.
Kev(Or I misunderstood the point)
Sam WhitedKev: 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.
flowSam Whited, so entities inspecting a stanza only have a linear stream of xml events available?
flowis it really constant memory use or *minimal* memory use?
Sam Whitedflow: 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.
Sam Whitedflow: 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)
Sam WhitedAnd it lets the user convert those into whatever representation they want.
flowcause I'd say once Smack has consumed an xml stanza the resulting java object instance also has constant memory use ;)
Sam WhitedThe 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.
Sam WhitedThat'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.
Sam Whitedoops, somehow I thought that errored the first time, sorry for the rephrase of the same thing.
Sam WhitedBut the memory use is really less interesting than just being low level and letting the user decode data however they want I think.
Sam WhitedAfter 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
Sam WhitedPushed an update with the new API and updated comments to explain it
Sam WhitedThanks for all the feedback, just talking through it out loud has been really helpful too.
flowAlways happy to act as rubber duck ;)
ZashHmm, rubber ducks as swag for FOSDEM and such?