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

put() stalls after suspend #5

Open
aaron97neu opened this issue Aug 13, 2020 · 9 comments
Open

put() stalls after suspend #5

aaron97neu opened this issue Aug 13, 2020 · 9 comments

Comments

@aaron97neu
Copy link

Previous discussion found here: oats-center/isoblue#60

Essentially, we were running into an issue where the oada.put() call would never return. This often occurred immediately after the ISOBlues came out of suspend. We believe that the issue stems from the server closing the websocket due to lack of "keep alive"s coming from the client, however when the client resumes from suspend, it does not realize this and continues using the now defunct websocket.

We are able to handle this pretty coarsely however some ability to check the status of the socket or force it to reconnect it would be helpful

@abalmos
Copy link
Member

abalmos commented Aug 13, 2020

@tomohiroarakawa Do you have any thoughts on how @oada/client could deal with this at the websocket level.

@awlayton Did you build in any keep-alive/check-if-alive message into OADA websockets that could be used (in your recent rewrite)?

I feel we may have similar issues after a prolonged cell outage as well, so this is not limited to ISOBlue's somewhat odd use case of surviving a system suspend.

@tomohiroarakawa
Copy link
Member

I probably need to look into the client WebSocket implementation, but if a browser reports disconnection events through ws.onclose or ws.onerror etc., then we should be able to protect put function from using non-existent WebSocket connection. I do not think oada/client is doing this type of error handling.

Automatic reconnection is a bit tricky. If the connection is already destroyed at the server, the client needs to not only create a new connection but also set up watches that previously existed. On the other hand, if the WS connection is lost but OADA connection ID is still active at the server, the client might be able to reuse the same connection ID. Of course, this requires some changes to the server though...

@awlayton
Copy link
Member

The websockets should be doing the ping pong thing.

@abalmos
Copy link
Member

abalmos commented Aug 18, 2020

@awlayton Are you saying that we are already doing ping/pong or that it may be the solution? I think @oada/client should just do that in the background. At least, if a certain time length has gone by without a real data packet.

Here is a reference: https://github.com/websockets/ws#how-to-detect-and-close-broken-connections

Maybe OADA should send a ping to any client that has not sent anything in N seconds and @oada/client should have a timeout if it has not received a message (of any type) in M seconds. Or clients could send the ping and the server timeout.

@awlayton
Copy link
Member

I meant the server already sends pings to clients.

@abalmos
Copy link
Member

abalmos commented Aug 18, 2020

Great, so we just need the client to timeout then. I think what we are seeing is the server has gone away but the client is not aware because the machine is asleep when the disconnect is issued.

The link above has an example of the needed client code. Might require some work to port for the ws wrapper that client uses.

@tomohiroarakawa
Copy link
Member

I believe most browsers do not expose the low-level WebSocket ping-pong APIs to a user. We might need to implement user-level ping-pong messages.

@abalmos
Copy link
Member

abalmos commented Aug 18, 2020

Also, this may be somewhat more complicated to make seamless. I suppose @oada/client needs to track watches, the current rev, and the callback to auto re-watch starting at the last rev received.

@tomohiroarakawa
Copy link
Member

Added timeout field to GETRequest, WatchRequest, PUTRequest, POSTRequest, HEADRequest, and DELETERequest in commits 0e5a962 and 64a90e3. See README for usage.

aaron97neu added a commit to oats-center/isoblue that referenced this issue Jan 11, 2021
Fixes new axios security issue axios/axios#3410.
Updates to client should also fix this: OADA/client#5
aaron97neu added a commit to oats-center/isoblue that referenced this issue Jan 19, 2021
Fixes new axios security issue axios/axios#3410.
Updates to client should also fix this: OADA/client#5
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

4 participants