tinkerpop-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [tinkerpop] FlorianHockmann commented on issue #1172: Handle closed connections gracefully instead of bubbling up exceptions
Date Thu, 02 Apr 2020 15:19:17 GMT
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

Mime
View raw message