Use of setTimeout with _processServerQueue dissociates thrown exceptions from data events

[asutherland]

first, apologies about being overly verbose here; I'm still wrapping my head around browserbox and I find it helps if I spell out what I'm thinking so my confusion can be addressed directly, but do feel free to skim/skip as appropriate :)

Context

In the Gaia email app we have a very limited attempt to imitate node.js's domain mechanism (http://nodejs.org/api/domain.html). We were planning to be fancier (see https://bugzil.la/1043516 and note that mcav's approach is a much simplified approach per discussion on IRC that is unfortunately not included in the bug), but we just ended up making sure that in our TCPSocket wrapper that if an exception gets thrown by the ondata handler that we kill the connection and we perform cleanup with the account that owns the connection.

We would like to maintain our fancy error handling at all times.

Analysis

Why setTimeout

It's not clear setTimeout is actually needed at this time. Its use seems to be an outgrowth of ImapClient.prototype._processServerQueue having callees that have async signatures but in practice are synchronous and call their callbacks before they return. So if setTimeout weren't used, we would expect that it's possible to blow the stack if _serverQueue has a lot of stuff in it.

Elaborating on the "stuff is actually synchronous", in ImapClient.prototype._processServerQueue there are two paths that appear to go async:

• calls to this._currentCommand.onplustagged
• calls to _processServerResponse

onplustagged appears to only be used for login and it synchronously invokes next().

_processServerResponse calls the following async-looking but not-async paths:

• _globalAcceptUntagged, every setHandler thing in browserbox.js calls next() during its handler
• this._currentCommand.callback, every call to .exec() in browserbox.js also seems to call next() synchronously in its handler.

This does beg the question of whether it would make more sense to have these functions just be synchronous and having _processServerQueue spin a loop. (Although it might sense to still yield control flow periodically.)

What about when streaming happens?

BrowserBox.prototype.listMessages is arguably a case where it would make sense to not buffer everything in its entirety before invoking the callback. I think this is probably orthogonal, however.

Specifically, it seems like the way to implement this would be to:

• allow imapHandler.parser to optionally take an optional handler that would be invoked as nodes are created/processed. In practice only literals are going to potentially be huge, but it could make sense for the parser to emit nodes as they happen. For literals over a certain size a stream would be emitted, otherwise a string would be emitted.
• have fetch support an EventEmitter mode of operation where it would re-emit those streams to the consumer. This could then be hooked up to other streaming-enabled parsers (mailreader) or directly streamed to disk, etc. Since I think the server is allowed to arbitrarily reorder its results so that it does horrible things like return BODY before UID and return the messages in a random order, it's possible browserbox might need to have a failsafe mode for pathological servers like that (sorta doubt they exist) where separate FETCH statements are issued for each UID so things can be inferred.

In the event the stream needs to involve some I/O that has setup overhead, the streams' backpressure mechanism can be used to propagate that back to the TCPSocket.

And doing this need not make any _processServerResponse calls async.

Proposal Options

make _processServerQueue sync, run-to-completion

It's not clear there's a benefit from the fake-async calls. Right now it seems to just be causing all the functions to have to call next() and potentially be a foot-gun if you leave one out. The idiom of callbacks being invoked while their function on the stack can also frequently be a foot-gun. A standard means of addressing this is to use Promises, but those are not a panacea and using them if you don't need them frequently just contributes to headaches.

I think there are some real simplicity benefits from just having the callbacks be synchronous. If there's concern about eventually needing the ability to be async and having to change the call signatures again, I would argue that generators should be sufficiently available at that point that the callee is required to just return/be a generator instead.

Allow nextTick to be passed in

When ImapClient is created let a nextTick function be passed in as an option for this purpose. If omitted, setTimeout(blah, 0) will be used by default. For Gaia email we can pass in our own wrapper that keeps things associated.

[asutherland]

Er, and I'm neutral on which option is best. It might make sense to do both, keeping nextTick around to allow other things a chance to interleave with your parsing. I'm assuming whiteout.io runs its IMAP logic in a background page like we run our IMAP logic in a Worker, so contention isn't likely to be a huge issue.

[andris9]

You can never be overly verbose 👍 I'll have to digest this through the weekend.

[asutherland]

hah, well, realized I wasn't verbose enough after all since I also left out part of the situation we're running into. Basically, we're seeing an error being thrown in listFolders while processing the results (I still need to investigate that a bit more and provide a fix, etc.), but it's being thrown and hitting the window's onerror handler instead of our catch{}. Presumably because of the use of setTimeout. (The onerror only gives us one stack frame, so the exact call-stack is not available.)

[felixhammerl]

hi andrew,

i totally agree with andris that you can never be overly verbose on a complex issue 👍
would it be ok for you if we scheduled a hangout where we can discuss this on a richer communication channel? github issues is a good way to track progress on an issue, but i think getting to know each other might improve the overall process and avoid misunderstandings on some issues that have come up recently.

cheers
felix

[tanx]

👍

[asutherland]

Sure thing. If you're around sometime in the next few hours I'd be fine to do it then/now, otherwise sometime where your convenient times overlap US East Coast works for me.

[felixhammerl]

sorry, we were just busy with reviews today, but what about tomorrow 09/24/2014 at 5pm CEST, 8am PDT?

@andris9 and @tanx will be joining, too.

[asutherland]

Sounds great. Just let me know what your preferred richer communications medium is; at Mozilla we can use our "vidyo" system with public URLs but I think it requires installation of a binary in all cases and its linux support is not great, so something else like skype or google hangouts or something webrtc-based is probably be better.

[felixhammerl]

alright, you should have an invite in one of your inboxes with a link to a hangout

[felixhammerl]

hey @asutherland, just wanted to inform you that @andris9 has started to migrate browserbox to promises. just thought you would be glad to hear that :)

[asutherland]

That's awesome!

[felixhammerl]

will be obsoleted by #72