MongoClient() updates, lock race fix, write concern support

[rocksfrow]

I've added logic to use MongoClient() if it's available, and to fallback to Mongo(). Along with that comes the now defunct 'safe' mode being used with Mongo(). Write concern/preferences are now used with MongoClient, and it fallsback to 'safe' mode with Mongo().

New config options:

connection_opts -- array of options to pass down to MongoClient() write_concern -- same default as MongoClient, 1, can be changed to 2+ to require writes to secondaries write_journal -- same default as MongoClient, false, can be enabled to require journal before ack

I also updated gc() so it uses w=0 instead of safe=false if using MongoClient.

I replaced ->save with ->insert in lock(), fixing #1. The lock is now using insert. I tested and confirmed the duplicate key exception gets thrown. The catch properly ignores that exception, but also now properly breaks out on any other exception (like write concern timeout).

I fixed a potential lock-out issue when a replication time out occurs. The class now calls unlock() to remove the partial writes (mongodb doesn't do this on it's own).

Updated dbInit() to use createIndex() instead of the defunct ensureIndex() for > '1.5.0'

[rocksfrow]

@nicktacular let me know if you have any questions at all

[rocksfrow]

@nicktacular any reason why no merge?

[nicktacular]

Hi @rocksfrow - sorry for the long delay. I've been very busy with other tasks at work. I'll review today and give you an update.

[rocksfrow]

BTW,

I have been running w my fork (w changes) without issue since.

The only issue remaining are locks getting stuck but I can't replicate.

Do you end up with records stuck in your locks table, slowly overtime?

I think I might update the garbage clean to clear old locks too, if I can't resolve why they're occurring. Any thoughts? On Sep 29, 2014 10:05 AM, "Nick Ilyin" notifications@github.com wrote:

Hi @rocksfrow https://github.com/rocksfrow - sorry for the long delay. I've been very busy with other tasks at work. I'll review today and give you an update.

— Reply to this email directly or view it on GitHub https://github.com/nicktacular/php-mongo-session/pull/5#issuecomment-57165231 .

[rocksfrow]

@nicktacular I guess you didn't get a chance to mess w/ this yet? Let me know if you have any questions/concerns.

I really would like to get this merged and move forward with remaining issues before I move the rest of my infrastructure over onto Mongo-powered sessions.

[nicktacular]

Hi @rocksfrow, sorry for the delay. My comments are inline.

General concerns:

• do not use tabs, please use spaces (4 spaces to each tab)
• there’s trailing spaces in a few parts, this should be cleaned up

[nicktacular]

Since this is a pretty general concern, I'll note this here as well.

Also, I’m wary of generating configs $this->instConfig[‘write_options’] here for 2 reasons. First, it doesn’t seem that you restore the original configs you set in the constructor anywhere that I can tell. Second, I would use a method to centralize write concern configs and document it so that it’s clear when and for what purpose you change configs.

Although I mention this on line 319, this has merit to be moved to a method that generates these and possibly restores when necessary, these configurations. It would make your code more DRY and easier to understand how you're generating these if it weren't everywhere in the class.

[rocksfrow]

@nicktacular, the spacing/tabs stuff is weird BC I didn't use tabs. Emacs auto indents with spaces but I'll get those cleaned up.

As far as a few of tour other concerns, I would agree. Most of those things were like that because I was fitting with your existing code. On Oct 7, 2014 9:20 AM, "Nick Ilyin" notifications@github.com wrote:

Since this is a pretty general concern, I'll note this here as well.

Also, I’m wary of generating configs $this->instConfig[‘write_options’] here for 2 reasons. First, it doesn’t seem that you restore the original configs you set in the constructor anywhere that I can tell. Second, I would use a method to centralize write concern configs and document it so that it’s clear when and for what purpose you change configs.

Although I mention this on line 319, this has merit to be moved to a method that generates these and possibly restores when necessary, these configurations. It would make your code more DRY and easier to understand how you're generating these if it weren't everywhere in the class.

— Reply to this email directly or view it on GitHub https://github.com/nicktacular/php-mongo-session/pull/5#issuecomment-58182572 .

[nicktacular]

As far as a few of tour other concerns, I would agree. Most of those things were like that because I was fitting with your existing code.

Not sure I understand. Can you give me an example?

[rocksfrow]

I've replied to your individual concerns.

I'll make these updates when I get around to it considering most of them are minor/style preference. It's weird tabs ended up in there though, I definitely prefer spaces over tabs too. I'll get those cleaned up no biggy.

Also, an example where I was trying to use your existing code logic would be (also previously commented on the line comment), where I was simply flagging the partial lock (lockAcquired = true), and then using the unlock function. This is an example where I just tried to use your existing code to resolve the issue rather than rehash a new function. I'll break the logic apart.

I'll get your change requests hashed out. Hopefully I don't wait a few months for your feedback next time :+1:

[nicktacular]

Regarding the style considerations, they should be fairly simple and quick to resolve.

Regarding the lockAcquired, I've looked through the code again and it appears that I'm just using that var in lock/unlock methods. My concern with your usage in that case was that you were specifying $this->lockAcquired = true followed by an unlock which seemed more of a hack, rather than the intended "just mark this as not locked since we couldn't lock", thereby making the change I suggested a clearer solution to solving this.

I appreciate your patience in this matter. I've been incredibly busy with work so my open source projects have suffered in lack of attention. I will do my best to ensure that I merge in your PR as soon as you've pushed the changes.

[rocksfrow]

@nicktacular

RE: write concern config changes

FYI, write concern was never meant to be changed during a session (my intentions i mean), but rather be set at time of init of the class. The only time write concern is overridden is internal to the class, and in error situations (to avoid a total lockout).

I can break it out into functions so it's more clear with the internal usage, but they will def be private functions, only used internally.

It shouldn't take long to make these changes, so thanks for giving it some time once I get the changes pushed.

[rocksfrow]

RE: lockAcquired "hack"

No it wasn't a hack. The lock does exist, just not on all nodes. I flagged lockAcquired = true, because unlock() was simply checking that condition before unlocking. In my opinion, adding a boolean $force to unlock is no less of a hack. The least "hackish" way would probably be to add another state, which is "partially locked", and update unlock() to handle partially locked sessions.

[nicktacular]

I was referring to how it was being done. Forcing an unlock makes sense in this context and is much clearer to the reader as to what is being accomplished here.

I look forward to seeing your updates.

[nicktacular]

I noticed #10 but it looks like it's a non issue?

[rocksfrow]

Yeah I closed it right out, it was an issue with my local copy (unpushed changes). On Oct 10, 2014 6:38 PM, "Nick Ilyin" notifications@github.com wrote:

I noticed #10 https://github.com/nicktacular/php-mongo-session/issues/10 but it looks like it's a non issue?

— Reply to this email directly or view it on GitHub https://github.com/nicktacular/php-mongo-session/pull/5#issuecomment-58724892 .

[rocksfrow]

@nicktacular I pushed up 4 commits that should resolve all/most of your "needed changes". Please review and follow-up ASAP while I have time to wrap this up.

[rocksfrow]

@nicktacular make that 5 commits (i missed you one comment).

I believe the only thing that would remain is the way that write_concern's are being handled. I honestly do not think that is something worth holding up this pull request for though.

Currently your class has some major limitations and can greatly benefit from the merge.

I can definitely change the way write_concern is being handled and make it a class var with accompanied functions like setWriteConcern and getWriteConcern... but the way it is implemented now isn't something to hold a pull request over IMO.

Let me know if any further things/questions. I'd like to get this merge wrapped up.

[nicktacular]

I'm not comfortable adding exception codes that aren't documented. Regardless of what other drivers do, the PHP driver doesn't have this documented anywhere that I can see: http://us1.php.net/manual/en/mongo.exceptions.php

I believe the only thing that would remain is the way that write_concern's are being handled. I honestly do not think that is something worth holding up this pull request for though.

It's been 15 hours since you've pushed a fix. Not sure I'm holding that up, but I had a legitimate concern from before. Thanks for addressing.

Currently your class has some major limitations and can greatly benefit from the merge.

Hyperbole isn't a the best way to get your pull request merged.

I can definitely change the way write_concern is being handled and make it a class var with accompanied functions like setWriteConcern and getWriteConcern... but the way it is implemented now isn't something to hold a pull request over IMO.

That's fine. Done is better than perfect.

I will merge right now and resolve style issues myself.

[rocksfrow]

RE: it's been 15 minutes

Umm... huh? Jul 29 this full request was created... I didn't push ANY fixes since then, only nuances that you wanted changed that probably would have taken you half the time to do than me (trying to guess how you want it). FYI, I have my emacs config updated to use spaces instead of tabs, so you don't have to worry about tabs coming through anymore.

RE: "hyperbole"

lol... ok bro. You have other people who even replied to the issue confirming the fix. I'm not exaggerating anything. I just don't like to see my time wasted. I won't waste it further.

RE: duplicate key codes

I did a lot of research on this, I didn't just look at one documentation page (yes I read the PHP doc of course) Like I said originally I was going to use the 11000 code, but after further research I found other possible codes that could be returned and it was iffy so I went with the regex to be sure. FYI, the 11001 is the duplicate key code from before mongodb 2.6. The third code I couldn't find documented, but I DID find it in multiple drivers, all being detected for the exact same duplicatekey exception. Also, I'd like to add that that condition is ONLY for backwards compatibility... any current release will throw the duplicatekey exception, and that second condition won't even get hit... it's there simply to support older installations. If you want to remove the third code I threw in there feel free, BUT, I did do research on that code and confirmed it wasn't a conflicting code anywhere.

Right from the PHP driver: "11001 and 12582 are also valid duplicatekey error codes"

https://github.com/nicktacular/php-mongo-session/commit/1d527b0e7c0b7bf41070cd6baac7cf77b5fc5a6c

RE: i'll fix style myself

THANK YOU. That's what you should/could have done 3 months ago :)

I've never seen somebody so unappreciative over contributions. You should be thanking me for my pull request, not talking down to me. Take it ez.

[rocksfrow]

To be clear... I LOVE to contribute... I'd just like a little appreciation at the same time!

[rocksfrow]

@nicktacular I hope you see my comments and fix you pushes ASAP... I am definitely wasting my time contributing to your class.

https://github.com/nicktacular/php-mongo-session/commit/1d527b0e7c0b7bf41070cd6baac7cf77b5fc5a6c

[nicktacular]

Hi @rocksfrow - when you say things like "I am definitely wasting my time contributing to your class" you're being completely and unnecessarily dramatic. Please, calm down, and we can finally get some work done together.

I would like to again, thank you for you time to contributing to this OpenSource project and for finding the various bugs and addressing them. Your time is really appreciated and valued.

[nicktacular]

Also, see 323470d for the latest commit reviving the code checks per your research.

[rocksfrow]

Hey man no problem. Appreciation goes a long way so when somebody puts hours of research into something and somebody else tosses it aside at a whim (without doing the proper research), I am sure you can appreciate the frustration. Not being dramatic. I'm glad I was able to finally convince you I knew wtf I was talking about Lol.

Thanks for writing this sweet class to begin with though! Look forward to more fun pull requests :) On Oct 28, 2014 10:34 AM, "Nick Ilyin" notifications@github.com wrote:

Also, see 323470d https://github.com/nicktacular/php-mongo-session/commit/323470de099551c38d57166b5c8182078f2d15e9 for the latest commit reviving the code checks per your research.

— Reply to this email directly or view it on GitHub https://github.com/nicktacular/php-mongo-session/pull/5#issuecomment-60764788 .

[nicktacular]

Absolutely, I appreciate the well thought research and all the fixes. I look forward to being more responsive to OpenSource efforts and any other contributions.