This may be related to #1306074 which ends up with :

So, returning as if nothing happened and just watchdogging is not a good idea. Throwing an exception is the right thing to do we just need to make sure that Drupal can handle it and there is not another exception thrown while at it. I have added code to make this happen.

.

Using mongodb_cache module and trying to shut down the mongodb server and see what happens. I get a crash:

Warning : file_exists(): Unable to find the wrapper "public" 
MongoConnectionException : Transport endpoint is not connected in Mongo->__construct()...

I think the right way to handle server unreachable in the case of the cache handler is to remove all the cache functionnality and alert the user (as this is a critical issue but the site can still work, so at least try to get the page result).

CommentFileSizeAuthor
#7 1941806-cache_set_failed-7.patch7.06 KBfgm
#6 1941806-cache_set_failed-6.patch5.43 KBfgm
#1 1941806-fix-for-mongocache-with-mongo-down-1.patch2.72 KBregilero

Comments

regilero’s picture

regilero commented
Status: Active » Needs review
StatusFileSize
new 1941806-fix-for-mongocache-with-mongo-down-1.patch2.72 KB

So here is the patch

bigjim’s picture

bigjim commented

I was about to submit something very similar. :)

+1 for this patch, works as expected.

regilero’s picture

regilero commented
Status: Needs review » Reviewed & tested by the community

So I move it to status reviewed.

chx’s picture

chx commented

Warning : file_exists(): Unable to find the wrapper "public"
MongoConnectionException : Transport endpoint is not connected in Mongo->__construct()...

Huhwaht? public not found...? I dont quite get it. I guess there hasnt been a lot of interest in the past because moft people usethe system in a way where no cache means no system anyways: iether sclaability reaons or entity is in mongodb anyways.

fgm’s picture

fgm
Primary language French
Location Paris, France
commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work 
+++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+  protected static $_connect_error = FALSE;

Need to explain why this is a static property. I think it's there because connection errors are expected to be shared between all bins, but technically we support having per-collection (hence per-bin) aliases, pointing to different MongoDB connections, so a connection error in one bin may not always imply a connection error in another one.

I'm not against making this restrictive hypothesis since it's likely to be the majority case, but it should be documented somewhere in the patch, possibly as a note in the MongoDB cache section of the README. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+      $collection = mongodb_collection($this->bin);

    Any reason to recompute the collection every time instead of keeping it as an instance property ? 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+      $cache = $collection->findOne(array('_id' => (string)$cid));

    Coding standards: spacing. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+        drupal_set_message(t('Mongodb Cache problem %except',array('%except' => $e->getMessage())),'error');

    Coding standards: spacing, array wrapping. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+        self::$_connect_error = TRUE;

    Might want to use static:: instead of self:: in case someone uses an inherited cache handler class. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+      $collection = mongodb_collection($this->bin);

    Same question about keeping the collection as a property to save function calls. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+      if(empty($result)) {

    Coding standards: spacing. 

  • +++ b/mongodb_cache/mongodb_cache.inc
@@ -17,35 +17,55 @@ if (!function_exists('mongodb')) {
+        drupal_set_message(t('Mongodb Cache problem %except',array('%except' => $e->getMessage())),'error');

    Coding standards: spacing, array wrapping.

    • fgm’s picture

    fgm
    Primary language French
    Location Paris, France
    commented
    Status: Needs work » Needs review
    StatusFileSize
    new 1941806-cache_set_failed-6.patch5.43 KB

    Rerolled. Core-equivalent tests pass.

    fgm’s picture

    fgm
    Primary language French
    Location Paris, France
    commented
    StatusFileSize
    new 1941806-cache_set_failed-7.patch7.06 KB

    Rerolled to also protect the garbageCollection(), set(), and isEmpty() calls. Core-equivalent tests still pass.

    • fgm committed 801c255 on 7.x-1.x 
      Issue #1941806 by regilero, fgm: Handle mongodb crash for mongodb_cache...
    fgm’s picture

    fgm
    Primary language French
    Location Paris, France
    commented
    Status: Needs review » Fixed

    No comments in over 4 months, tests still pass on current HEAD, assuming OK. Committed to today's HEAD.

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.