[JAVA-1579] Unclear API Created: 01/Dec/14 Updated: 31/Jan/18 Resolved: 31/Jan/18 |
|
| Status: | Closed |
| Project: | Java Driver |
| Component/s: | API, Documentation, Error Handling |
| Affects Version/s: | 3.0.0 |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Daniel Gottlieb (Inactive) | Assignee: | Unassigned |
| Resolution: | Won't Fix | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Description |
|
I was under the impression that catching `MongoException` was sufficient for error handling driver calls to the database. In cases of network/mongo instability, I'm also seeing `IllegalStateExceptions` which is escaping our normal error handling. Would it make sense to have all exceptions extend from `MongoException`? If not, is it possible to publicly document a definitive list of high-level exceptions that can come out of mongo driver calls that all applications are recommended to catch? Is that list limited to `MongoException` and `IllegalArgumentException`? If so, can we guarantee nothing new will be added that doesn't fit that classification or is it recommended for applications to catch `Exception`?
|
| Comments |
| Comment by Jeffrey Yemin [ 31/Jan/18 ] | ||||||||||||||||||||||
|
As no other users have voted this up in the past three years, closing as Won't Fix. | ||||||||||||||||||||||
| Comment by Dana Groff [ 30/Aug/16 ] | ||||||||||||||||||||||
|
jeff.yeminPlease make a decision if we are going to do this or not. It's pretty old so we should just make a decision. | ||||||||||||||||||||||
| Comment by Daniel Gottlieb (Inactive) [ 03/Dec/14 ] | ||||||||||||||||||||||
|
`MongoClient` objects are expected to be shared throughout the application and while it is user error to close a `MongoClient` that a distant thread is still using, that's a very likely scenario to happen due to the global nature of `MongoClient`s. It's still fair to wave the "user error" flag in response, but the API should at least meet the demand halfway and commit to and document the exception that is expected to be thrown in that case. A sample function, noting the substitution of catching a `MongoException` with an `Exception`.
I make no claims as to what trivial cases of surface level user error like inserting null should result in. But non-trivial, detected, user error of incorrect usage (e.g: incorrect internal state due to a race) should result in a specified error. Moreover, that exception really should be a `MongoException` such that the above code doesn't look silly. It's harmful to make a distinction that user errors that can only be detected deep within the driver code from are inherently different from other "pure" mongo errors that can only be detected deep within the driver code. Even if a `collection.isMongoOpen()` method existed and the sample code used it, there's no protection offered against a careless coder that introduces a bug (incorrect `close`) far away in the application. | ||||||||||||||||||||||
| Comment by Jeffrey Yemin [ 02/Dec/14 ] | ||||||||||||||||||||||
|
You raise good points, and I agree that the driver should do better at documenting the exceptions that can be thrown for each method, but I'm curious what a catch block for an IllegalArgumentException might look like. If the driver throws IllegalArgumentException it means that the calling code has a bug (or else is using exceptions as flow control, which is another matter), and the most sensible thing for it to do is catch Exception at a very high level and log it, as it requires human intervention to determine what went wrong. (For this particular case, a good argument can be made that the driver should not throw at all, and instead allow the in-progress operation to complete normally even if the MongoClient is closed in the middle. In other words, the IllegalStateException is unintentional and should be considered a bug, not documented. I'm going to open a separate ticket to address this.) | ||||||||||||||||||||||
| Comment by Daniel Gottlieb (Inactive) [ 02/Dec/14 ] | ||||||||||||||||||||||
|
That's in the realm of possibility (though it's hard to understand where the close is being called from given the whole "MongoSvc" thing in MMS) and I agree that's user error, but I would still argue that the function needs to be documented on what it can throw (with the possible exception of say the user trying to insert a null document or calling `DB.getCollection(null)` sort of things, that's more of a matter of taste that I have little opinion on). So long as the `DBCollection.insert` method can throw an `IllegalStateException` (which does not seem to be an `IllegalArgumentException` despite the function declaration in `Assertions.isTrue`), it should at the very least be documented on `DBCollection.insert` (as well as the other methods). Making a stronger statement, best practices of catching errors inside calls to mongo should be probably be documented as catching `catch (MongoException | IllegalArgumentException | IllegalStateException exc)` or for the lazy (pragmatic?) `catch (Exception exc)` (or `RuntimeException`?). I would argue anything that the driver code is explicitly checking and throwing needs to be documented and future API compatible versions should not break that contract by adding anything that isn't caught by existing documented (runtime)exceptions. It's not good that the driver is coy about how it can (oftentimes unintentionally) unwind the application execution stack. | ||||||||||||||||||||||
| Comment by Jeffrey Yemin [ 02/Dec/14 ] | ||||||||||||||||||||||
|
In general this is going to be hard to achieve, as the driver may throw various RuntimeException subclasses, in two main circumstances:
This particular case seems like a variant of the first case, and it would be good to understand what's going on. The critical code block is this one: https://github.com/mongodb/mongo-java-driver/blob/7a970716d359078438a933c0093d20e718a13a0f/src/main/com/mongodb/DBCollectionImpl.java#L184-199. The first call in the block is to DBTCPConnector.getPrimaryPort(). The first thing that method does is check that the DBTCPConnector instance is open, and clearly that check succeeds: https://github.com/mongodb/mongo-java-driver/blob/7a970716d359078438a933c0093d20e718a13a0f/src/main/com/mongodb/DBCollectionImpl.java#L184-199 The DBTCPConnector is essentially just the MongoClient, so this open check is ensuring that the MongoClient is still open. The last call is in the finally block, and it's a call to DBTCPConnector.releasePort(). That method also checks that the DBTCPConnector instance is open, and in this case the check fails, which means somewhere in between these two calls the application appears to have closed the MongoClient instance that is being used by the BlockStoreDao instance. Is this in the realm of possibility? Are you seeing this exception logged around the time that an application shutdown is in progress, which may be closing all MongoClient instances while operations are still in progress? |