#6: Fix up error handling

[charles-dyfis-net]

• Errors are now written to both the read channel and the channel-delivery channel (in the latter case, wrapped in a unidirectional, single-purpose channel which delivers that error and has no other use).
• Do not assume that on-error will return an event with a data attribute. This attribute is not guaranteed to exist by the W3 spec for WebSockets. Instead, defer error delivery until onclose() is called, which is guaranteed by spec to have reason and code attributes.
• Do not assume that put! will act synchronously.

[charles-dyfis-net]

This one now looks pretty good on my end (after rather a lot of interim revisions). Feedback appreciated.

[charles-dyfis-net]

...note: on current dev-channel Chrome, no data attribute is available on error objects, and the reason attribute for a connection-failed operation is frequently an empty string.

This appears to be by design and intent, and is likely to be the case on other browsers in the future if not already so -- best practices for WebSockets implementations suggest restricting information about the nature of a connection or negotiation failure, to prevent websockets from being used to implement port scanners.

[jarohen]

Hi Charles,

Thanks for all your work on this, I really appreciate it :)

I should have chance to look over the PR tomorrow, will let you know!

James

[jarohen]

Thanks for the PR - I've got some initial feedback and some thoughts:

• I'd recommend splitting out your commits to individual bugs/features - this really helps with figuring out what the PR as a whole is doing because you can look at and reason about each commit separately. (Some project maintainers ask for a separate PR per bugfix/feature - I'm not so fussed about this but it definitely helps to get distinct diffs). I'd probably have split out the try-read-* fixes into a separate commit (good find, btw, thanks!), and also the 'put! being asynchronous' fix (likewise, thanks!)
• I'm not entirely clear about the current code-flow - especially with regard to the .-error-seen variable - if mutable state is necessary, I usually try to avoid needing to reason about it beyond the confines of a single function. In any event, it's probably a good idea to steer away from mutable variables at all if we can avoid it, especially in amongst all the asynchronicity.
• I've not previously read the WebSocket spec as meticulously as I should have - thanks for taking the time to do this! Having done so now, (if I understand correctly), the JS websocket error event is always followed by a close event, and the error event can't contain any 'useful' information, as you say - yet the close event has reason, code, and wasClean. This seems to suggest that we may not need to worry about the onerror handler, and it may be introducing complexity/state that we don't need. Am I on the right lines here?
• Your PR has thrown up an idea about the overall structure of the CLJS code - given that we're fundamentally dealing with an abstraction layer over a state machine, I might be tempted to architect Chord as a state machine - i.e. one go-loop in control of the various states/events - it would probably make the state of the system a lot clearer. What do you think?

Thanks again,

James

[charles-dyfis-net]

Howdy, James --

Thank you for the time and feedback.

• I'd be happy to split the commits by feature; look for that earlier today.
• I agree that .-error-seen can go away -- its existence was intended primarily to maintain support for any pre-standard implementations you had previously tested developed and tested against. With it, I further concur that .onerror can be removed.
• The only potential concern I may have about a state-machine implementation is that the approach that strikes me as most natural would mean changing the public API from what's currently available. Unless you have something particular in mind, perhaps this should be an 0.4 feature?

[jarohen]

To be honest, I think it's going to have to be a 0.4 release anyway - one of the changes I'd like to make is that the initial channel returns either {:ws-chan chan} or {:error error} - otherwise I don't think we'd be fixing the original problem ;)

Having said that, hopefully we can keep the state machine implementation far enough away from the public API s.t. there's no breaking changes caused by the change in the internals - will see if it's possible...

[charles-dyfis-net]

I can't argue with you that passing back a channel which yields an error and then closes could be a bit misleading if the user has been promised that any yielded channel passed will represent an open socket; the approach presently taken in this PR is an attempt to do what we can within the bounds of compatibility with the existing API.

If we're willing to lose the ability to pass back such metadata as is available (which, given how little metadata is available about connection failures to deter port scanner development, I'd argue is not much loss), simply closing the initial channel rather than passing back any return channel would be a significant improvement on existing behavior which would avoid breaking any promises made by the 0.3.x API, allowing 0.4.x (with its breaking changes, and more detailed error messages) to follow later.

[jarohen]

Even closing the channel would be a breaking change though - i.e. the channel yielding nil rather than not yielding a value. It's quite reasonable for callers in 0.3.x to assume that (<! (ws-ch "...")) will never return nil (given that, currently, it will hang - at the moment it's up to the caller to wrap it in a timeout, which obviously isn't ideal!). For example:

(go
  (let [socket (ws-ch "ws://localhost:3000/ws")
        [v c] (a/alts! [socket (timeout 2000)])]
    (if (= c socket)
      (js/console.log (<! v))
      (js/console.log "timed out"))))

is perfectly valid under 0.3.x, but would break if we closed the initial channel on error.

I'd prefer to call such a release 0.4.x even if the breaking change is slight, just in case someone does depend on this behaviour - better to flag it and document it in the changelog than have someone upgrade an incremental version and get an NPE-like surprise.

[jarohen]

In other news, I've made a few other changes (mostly things I've been meaning to do for a while!):

• I've refactored out code that can be shared by client and server into shared CLJX code - this is mainly to do with the formatting of the messages (in format.cljx) and a little bit of shared core.async code (in channels.cljx)
• I've refactored out the CLJS in the example project (there was an awful lot happening in one file) - and commented it more thoroughly
• I've also refactored chord.client to a simple state machine (not quite as complex as I'd envisaged, thankfully!)

The changes are on the 'connection-error-handling' branch in my repo.

James

[charles-dyfis-net]

Howdy --

I've updated my client code to use 0.4.0-SNAPSHOT, as implemented in the connection-error-handling branch, and it appears to play well. (Reworking the PR seems a dubious endeavor now that there's a substantial amount of new work developed on top of it; is this still something you'd like to see?)

That said, it looks like the [org.clojure/tools.reader "0.8.4"] dependency here conflicts rather badly with clojurescript 0.0-2202 (which depends on 0.8.3, and generates code which fails at runtime with 0.8.4).

[jarohen]

I've updated my client code to use 0.4.0-SNAPSHOT, as implemented in the connection-error-handling branch, and it appears to play well. (Reworking the PR seems a dubious endeavor now that there's a substantial amount of new work developed on top of it; is this still something you'd like to see?)

That's good :) Yes, agreed - no need to worry. I'll cut an rc1 today and sanity test it on a few of the projects that use it. Think it's then just the ChangeLog that needs updating, and we're good for a stable release.

That said, it looks like the [org.clojure/tools.reader "0.8.4"] dependency here conflicts rather badly with clojurescript 0.0-2202 (which depends on 0.8.3, and generates code which fails at runtime with 0.8.4).

I'm never sure whether to include a tools.reader dependency - sometimes it's required, sometimes a specific version is required, sometimes it doesn't matter, sometimes its inclusion breaks things horribly... sigh

I think Chord does need a tools.reader dep - if only for the people who only use the CLJ side without a ClojureScript dependency - the question is 'which one?'. I'll try out 0.8.3 as you suggest and see what happens!

Thanks,

James

[jarohen]

The fix to use 0.8.3 seems to be fine - I've just deployed a 0.4.0-rc1 release.

Could you let me know whether this works for you?

Thanks,

James

[charles-dyfis-net]

The release candidate works nicely. :)

[jarohen]

Excellent, thanks - I've just deployed 0.4.0.

Thank you for all your help with this release, it's much appreciated!

James