Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create synthetic onclose events from disconnected chsk's #255

Closed
danielcompton opened this issue Aug 5, 2016 · 14 comments
Closed

Create synthetic onclose events from disconnected chsk's #255

danielcompton opened this issue Aug 5, 2016 · 14 comments
Assignees

Comments

@danielcompton
Copy link
Collaborator

I'm trying to determine if a chsk has been closed manually by chsk-disconect! when looking at it's state. It seems like there are some differences in behaviour between connected and disconnected chsk's when chsk-disconnect! is called:

If a ws connection is connected, and chsk-disconnect! is called on it, the WebSocket onclose event will be called, and the state of the connection will be updated to :open? false, and include an onclose event which contains :clean? true.

If a ws connection is disconnected and trying to reconnect, calling chsk-disconnect! on it will set the state of the connection to :open? false, but no onclose event will be called.

Is there a reliable way of determining when chsk-disconnect! has been called on a chsk, other than wrapping this call myself?

@ptaoussanis
Copy link
Member

Sorry, just juggling some urgent tasks so only have a moment to look at this-

Is there a reliable way of determining when chsk-disconnect! has been called on a chsk, other than wrapping this call myself?

To clarify- you're trying to distinguish between when a channel socket is closed because you called chsk-disconnect! and when it's closed because it closed unexpectedly? I.e. you want the reason its disconnected?

Would need to think about that, the manual disconnect was something I never gave much thought to and it could probably be improved. Let's start by confirming exactly what functionality you'd like.

Thanks, cheers! :-)

ptaoussanis added a commit that referenced this issue Aug 5, 2016
…nections

Channel socket state now contains a `:last-close-cause` key with value
e/o #{:requested-disconnect
      :requested-reconnect
      :downgrading-ws-to-ajax
      :unexpected}

BREAKING:
  - `:requested-reconnect-pending?` key has been dropped (undocumented)
@ptaoussanis
Copy link
Member

Had a moment so took a quick stab at something you might find useful (?) at c90fccc

Completely untested, and I'm not sure if these are the kind of semantics you're looking for. In any case have wanted to generalize the state merge stuff so this'll be useful for that either way.

@danielcompton
Copy link
Collaborator Author

To clarify- you're trying to distinguish between when a channel socket is closed because you called chsk-disconnect! and when it's closed because it closed unexpectedly? I.e. you want the reason its disconnected?

Exactly, we want to know if it's closed because of something we did, or closed for a reason outside of our control. The experimental commit looks like it has what we're after. Thanks!

@ptaoussanis
Copy link
Member

Just pushed [com.taoensso/sente "1.11.0-SNAPSHOT"] to Clojars. When you have some time, may I ask you to confirm that this is resolved?

Added relevant documentation here.

Thanks

@danielcompton
Copy link
Collaborator Author

This is pretty good. The only thing I would change is to add :last-close as soon as the connection closes as well, i.e. when there is a closed socket, we want to know then why it is closed, not later when it is reconnected, or even worse on the second time it was disconnected, the reason from the first time.

@ptaoussanis
Copy link
Member

The intention was to operate exactly as you described, is it not working that way now?

@danielcompton
Copy link
Collaborator Author

Sorry, I was getting my wires crossed from my testing. This works fine once you've been able to make a connection, but won't work if you are never able to make a connection. But this is one place where nil punning makes sense to do :)

I think I'm happy with this. I'll do some testing as I integrate the changes, but they look pretty good to me.

@ptaoussanis
Copy link
Member

The logic currently goes: update :last-close any time :open? changes from true to false.
If the connection's never been established, there won't (cannot) be a cause of closing; in that case you might want to look at :last-ws-error, etc.

Does that make sense?

@danielcompton
Copy link
Collaborator Author

Yep, makes sense. I just remembered what the issue was. One use case we have is to disconnect a websocket if it has been disconnected after some amount of time. The current approach won't work for identifying the state of the chsk after the disconnection, as it will have originally been disconnected for an :unexpected reason, and then shut down manually later.

Another use case we have is to shut down the chsk if we couldn't establish an initial connection after some timeout. Here again, we need to be able to determine the difference between a websocket that was stopped by our timeout checker, vs a websocket that is disconnected but will be retrying.

I don't fully understand the :downgrading-ws-to-ajax logic, but I suspect this might also have the same issue.

I think the intuition I'm after is "I can tell why this chsk is disconnected". :unexpected events should only be added on the transition from open to closed, as there is only one disconnection event, even though there may have been many WebSocket reconnections made. However, I think that the other three events should be updated even if the chsk is already closed?

Perhaps something like this?

diff --git a/src/taoensso/sente.cljc b/src/taoensso/sente.cljc
index c9d2a21..1dbc8d9 100644
--- a/src/taoensso/sente.cljc
+++ b/src/taoensso/sente.cljc
@@ -806,41 +806,42 @@
 #?(:cljs
    (defn- chsk-state->closed [state reason]
      (have? map? state)
      (have? [:el #{:requested-disconnect
                    :requested-reconnect
                    :downgrading-ws-to-ajax
                    :unexpected}] reason)
-     (if (:open? state)
+     (if (or (:open? state) (not= reason :unexpected))
        (assoc state
          :open? false
          :last-close {:uuid (enc/uuid-str) :reason reason})
        state)))

@ptaoussanis
Copy link
Member

One use case we have is to disconnect a websocket if it has been disconnected after some amount of time. The current approach won't work for identifying the state of the chsk after the disconnection, as it will have originally been disconnected for an :unexpected reason, and then shut down manually later.

Any chance I could ask you to rephrase this as a linear progression of events, and point out the event where you'd like the current behaviour to change?

@ptaoussanis
Copy link
Member

I think the intuition I'm after is "I can tell why this chsk is disconnected".

Juggling a lot of stuff right now so may be missing something, but feel like you should be able to do that with the current implementation?

  • If :last-close is :unexpected, then a previously established connection broke abnormally.
  • If :last-close is anything else, then a previously established connection was intentionally disconnected.
  • If :last-close is nil, then you've never been connected and you'll want to look at :last-ws-error.

Am I missing something?

@danielcompton
Copy link
Collaborator Author

danielcompton commented Aug 8, 2016

Sure:

  1. Connection is established
  2. Some time later, the connection drops for an unexpected reason, the state will have :last-close :unexpected
  3. Our app notices we are disconnected and starts a watchdog timer for 1 hour
  4. After 1 hour we are still disconnected, so the watchdog timer disconnects the connection.
  5. At this point :last-close should now be :requested-disconnect, but it is still :unexpected

@ptaoussanis
Copy link
Member

Sure:

Awesome, thanks. That was much easier for a tired mind to digest :-) Your suggested change sounds good, will update.

ptaoussanis added a commit that referenced this issue Aug 8, 2016
… reason is available (@danielcompton)

Defining "better" here as expected > unexpected.

Motivating example:

1. Connection is established
2. Some time later, the connection drops for an unexpected reason, the state will have :last-close :unexpected
3. Our app notices we are disconnected and starts a watchdog timer for 1 hour
4. After 1 hour we are still disconnected, so the watchdog timer disconnects the connection.
5. At this point :last-close should now be :requested-disconnect, but it is still :unexpected
@ptaoussanis
Copy link
Member

Done, updated Clojars snapshot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants