xmppjs / xmpp.js

XMPP for JavaScript
ISC License
2.19k stars 372 forks source link

Exception during client stop #805

Closed wichert closed 4 years ago

wichert commented 4 years ago

When our ReactNative app wakes up (background -> foreground app state transition) we restart XMPP to make sure we have a valid fully working connection as soon as possible. The flow looks like this:

  1. application state switches to background
  2. We call client.reconnect.stop() to stop reconnect attempts
  3. We change our presence to unavailable
  4. Wait until application state switches to foreground
  5. We call client.stop() and client.start() to make sure we quickly have a working connection, without having to wait for a ping test.

Occasionally this will throw an exception. When that happens to callstack looks like this:

Which means that the parser for the connection was set to null.

I'm not quite sure if stopping and starting the client is the best way to make sure we get a working connection quickly. Is there a better way to handle that ?

sonnyp commented 4 years ago

There are 2 independant topics here

Regarding the exception, you should investigate why it happens. In theory client.stop() cannot throw exception see https://github.com/xmppjs/xmpp.js/blob/master/packages/connection/index.js#L205

Regarding connecting quickly, you probably want to look into stream resumption https://github.com/xmppjs/xmpp.js/pull/776

I'm not quite sure if stopping and starting the client is the best way to make sure we get a working connection quickly. Is there a better way to handle that ?

IIRC the reason crypho does not call stop when going to background is because that wouldn't work reliably on iOS due to timers being registered. It was a while ago meanwhile iOS, React Native and xmpp.js changed quite a bit so you might want to reconsider, altough I'd be very careful with changing the logic as the way I left it was the result of quite a lot of debugging and tweaking to get something working reliably.

wichert commented 4 years ago

I made a small test app to test behaviours. One thing I can see is that if you quickly try to switch credentials you get an [TypeError: null is not an object (evaluating '_this5.socket.write')] error. Looking at the logs this happens if you try to do xmpp.restart() (to start a new stream with new credentials) while the current state is opening. This is easily reproducible by just pressing the button a few times.

sonnyp commented 4 years ago

@wichert you're misusing restart, it doesn't do what you think it does.

See https://github.com/xmppjs/xmpp.js/blob/master/packages/connection/index.js#L315

wichert commented 4 years ago

How is that misusing restart? The goal is to restart the stream with new credentials, which seems exactly what the function is supposed to do.

wichert commented 4 years ago

If that is misusing, what is the right way to switch credentials?

sonnyp commented 4 years ago

What are you trying to achieve? Try multiple passwords? Reconnect with a different user?

wichert commented 4 years ago

Reconnect with a different user.

sonnyp commented 4 years ago

@wichert just call start then

wichert commented 4 years ago

And await xmpp.stop() before that? start() will throw an exception if the current status isn't offline.

sonnyp commented 4 years ago

@wichert right that should work well

Please note that xmpp.stop() will also stop auto reconnect so you probably want to try something like:

await xmpp.stop()
xmpp.reconnect.start()
await xmpp.start()
wichert commented 4 years ago

The documentation for start() suggests that reconnect is started automatically:

Starts the connection. Attempts to reconnect will automatically happen if it cannot connect or gets disconnected.

wichert commented 4 years ago

I can't find any logic for xmpp.stop() that disables reconnect (other than reconnect itself checking for status==='offline'). Are you sure xmpp.stop() stops reconnect?

sonnyp commented 4 years ago

other than reconnect itself checking for status==='offline'

That's what "stops" reconnect.

The documentation for start() suggests that reconnect is started automatically:

Yeah it's not very clear. start doesn't enable reconnect, it's just that reconnect is enabled by default.


I guess either the documentation should be made clearer but behavior would still be confusing IMHO or start should just "enable" reconnect.

sonnyp commented 4 years ago

@wichert my bad, reconnect doesn't "disable" when you call stop(). Got confused with something else.

However because of timer issuers on iOS you might have to disable/enable it when app state changes.