[GitHub] [tinkerpop] FlorianHockmann commented on issue #1172: Handle closed connections gracefully instead of bubbling up exceptions

[GitBox <gi...@apache.org>]

FlorianHockmann commented on issue #1172: Handle closed connections gracefully instead of bubbling up exceptions
URL: https://github.com/apache/tinkerpop/pull/1172#issuecomment-607910571
 
 
   This PR has been open for quite some time and I would like to get back to the underlying issue @WiredUK wanted to address here.
   Since the discussion thread here is quite long, I'll try to summarize the current state first so we're all on the same page:
   - The problem was that the driver threw a lot of `NullReferenceException` when `ReceiveAsync` was called on the `WebSocketConnection` while it was not in the `Open` state. Otherwise, the fix to check whether the state is open before calling `ReceiveAsync` wouldn't help.
   - @WiredUK mentioned that this happens in just a few seconds after starting his application. So, this is not an issue of Cosmos DB closing connections after a while (as described in [TINKERPOP-2288](https://issues.apache.org/jira/browse/TINKERPOP-2288)).
   - The driver [changed in the meantime](https://github.com/apache/tinkerpop/pull/1250) to now throw a clear message when it received `null` / an empty array.
   
   Is that summary correct or did I miss anything important or got something wrong?
   
   While reading the comments again, I wonder whether this one actually points to a good possible solution here:
   
   > Maybe, but then the code gets "stuck" inside the while(true), which is a tight loop, waiting for the external connection check, and only exits when an exception happens.
   
   So, why not simply replace `while(True)` with `while(IsOpen)` in `Connection.ReceiveMessagesAsync`? If the issue here is that we try to receive messages on a closed connection, then this is the easiest fix in my opinion as we already have the information about the connection state in the `Connection` class. So we don't need to introduce a status object and return that from our `WebSocketConnection` class.
   
   The change will actually be a bit more complicated as we need to cancel open requests which results in an exception for the user but I think that this is in general an easier solution to stop calling `ReceiveAsync` on a closed connection.
   
   What I'm however still wondering about is why the connection is not in the `Open` state in the first place. Was it closed by the server? Or by the client? Or is the connection maybe not even open yet when we call `ReceiveAsync` the first time which would explain why @WiredUK sees the exception even within seconds of starting his application.
   The last case would be very surprising to me as we're calling `ReceiveAsync` only after `await`ing for `ConnectAsync`.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services