Adding a custom session handler with Cloud Datastore. #213

tmatsuo · 2016-10-21T01:02:18Z

It's great if you give early feedback!

src/Datastore/DatastoreSessionHandler.php

+
+    public function open($savePath, $sessionName)
+    {
+        $this->datastore = new DatastoreClient(


tmatsuo · 2016-10-21T20:35:36Z

Added a constructor :)

I will also add test. Maybe I can add unit test with mocking. Do you want to have a system test as well?

tmatsuo · 2016-10-22T02:54:11Z

Added the unit test.

tmatsuo · 2016-10-25T21:03:12Z

Maybe we should not ignore the $savePath and the $sessionName.

How about to use $savePath for the Datastore namespace and $sessionName for the Datastore kind?

tmatsuo · 2016-10-25T21:29:10Z

I started to use $sessionName for the kind.

However, utilizing $savePath while allowing the constructor injection of the DatastoreClient is tricky because there's no way changing the namespace afterwards. Do you have any recommendations?

dwsupplee · 2016-10-25T22:55:10Z

Looking really nice @tmatsuo, thank you for this.

While it is not documented , it is possible to pass a namespaceId through when constructing a key. ( @jdpedrie do you think it would be worthwhile to add documentation for that?)

$key = $datastore->key('key', 1234, [
    'namespaceId' => 'Namespace'
]);

The idea of using the $savePath to set that namespace sounds pretty slick to me.

A few comments:

Have you considered a form of locking at all?

I'm a little concerned about the garbage collection. According to the docs gc is called randomly. That could lead to some unexpected charges for users if they aren't familiar with the way this is configured to work. Do you think it would make sense to document that, or provide an alternative way for users to perform the garbage collection on their own terms?

src/Datastore/DatastoreSessionHandler.php

+                sprintf('Datastore lookup failed: %s', $e->getMessage()),
+                E_USER_NOTICE
+            );
+            return '';


tmatsuo · 2016-10-25T23:09:00Z

The idea of using the $savePath to set that namespace sounds pretty slick to me.

A few comments:

Have you considered a form of locking at all?

Good point. Maybe we can use the Datastore transaction? But it is a bit difficult with the current interface design. I think there are the same issues with any other session implementations.

I'm a little concerned about the garbage collection. According to the docs gc is called randomly. That could lead to some unexpected charges for users if they aren't familiar with the way this is configured to work. Do you think it would make sense to document that, or provide an alternative way for users to perform the garbage collection on their own terms?

Good point. Just for reducing the deletion cost (I think it's cheaper leaving entities than deleting them), maybe we can stop querying when gcLimit == 0 and document that. How does it sound?

Also, when I deployed a sample app with this handler, App Engine health check is constantly creating a new session entities in the datastore. We should also recommend that users use this handler only for needed handlers.

tmatsuo · 2016-10-25T23:09:52Z

While it is not documented , it is possible to pass a namespaceId through when constructing a key.

Also, is it same for query (which is needed in gc)?

jdpedrie · 2016-10-26T00:02:40Z

@tmatsuo yep, I added the ability to set a namespaceId on Datastore::runQuery() and Transaction::runQuery() in #214. It's not in a release yet, but you should have no trouble against dev-master.

jdpedrie · 2016-10-26T00:03:55Z

@dwsupplee I left it out of the documentation purposely because the right way to do it really is in the config. Most applications would want to use the same namespace across an entire project. If you think we should document it with a caveat I'm cool with that too.

tmatsuo · 2016-10-26T01:46:12Z

@jdpedrie Ah OK, great, I'll try to use that.

BTW, another viable solution for the session in cloud is to use Google Cloud Storage with gcs stream wrapper. I will also try to implement the stream wrapper.

Thanks,

dwsupplee · 2016-10-26T17:49:49Z

But it is a bit difficult with the current interface design. I think there are the same issues with any other session implementations.

Actually, I believe the default session handler locks.

Just for reducing the deletion cost (I think it's cheaper leaving entities than deleting them), maybe we can stop querying when gcLimit == 0 and document that. How does it sound?

That sounds like a solid idea. I wonder if it would be best to default the limit to 0 so that a user has to explicitly ask for the query/deletion to occur. What do you think?

tmatsuo · 2016-10-26T18:41:58Z

Actually, I believe the default session handler locks.

I see. There's no exclusive lock in datastore. Maybe we can create a transaction on open and commit in write?

That sounds like a solid idea. I wonder if it would be best to default the limit to 0 so that a user has to explicitly ask for the query/deletion to occur. What do you think?

Yeah I will do that.

tmatsuo · 2016-10-26T19:18:38Z

Now it uses the session.save_path for the namespaceId, and also uses transaction for data consistency. I still have a little concern on what happens when collision happens. Presumably it fails on write so a PHP_NOTICE error will be triggered, and usually you can see them in your logs.

Does it sound reasonable?

src/Datastore/DatastoreSessionHandler.php

+    public function open($savePath, $sessionName)
+    {
+        $this->kind = $sessionName;
+        $this->namespaceId = str_replace('/', '_', $savePath);


src/Datastore/DatastoreSessionHandler.php

+    {
+        $this->kind = $sessionName;
+        $this->namespaceId = str_replace('/', '_', $savePath);
+        $this->transaction = $this->datastore->transaction();


dwsupplee · 2016-10-26T20:55:10Z

Hmm, I don't think we would just want the write to fail if we run into concurrency issues. I will do a little more research after having pulled down and played with the code a little bit.

jdpedrie · 2016-10-27T22:27:37Z

A session handler that requires more than register, call session_start() is more than we should ask of users. It should be a drop in replacement for the default handler. Of course error handling is always a good thing, but expecting people to implement it on a part of their code that's expected to "just work" is another matter.

Yes, because write can also fail with other reasons. Only way to make sure whether the write succeeds is to have the error handler (or try-catch if we change it to throw exceptions).

Everything can fail, and obviously people should handle issues that can arise. That said, errors in the session handler are exceedingly rare. I can't recall ever running into one in the default implementation.

I'd rather not accept an implementation which we know can fail in certain (not uncommon) circumstances. Documenting how to work around those errors doesn't strike me as a compromise that is in the best interests of our users.

I think a Datastore session handler is a great idea, but as always the devil is in the details. As a user of the library, I would be worried about a session handler which was expected to fail in certain cases and which could very well cause problems for my users.

tmatsuo · 2016-10-27T22:33:19Z

@dwsupplee
Fair point, but as long as we use Datastore as the session store, it is still fair to say keep the write operation as few as possible in terms of reducing the cost :) Which also helps to avoid contention.

@jdpedrie
The default file implementation can fail too. Any other network based session implementations will fail more often of course due to network errors. Ignoring those errors is OK for some use cases, but I still think handling errors for session's write is a good practice with the current design of PHP session, especially if you don't want to lose the data.

But yeah, If you are not comfortable with this handler, I will just put it somewhere else :)

tmatsuo · 2016-10-27T23:00:41Z

In other words, all the peculiarity you feel from this handler, comes from the characteristic of the Datastore API itself. It provides the data consistency feature via it's transaction. It is optimistic lock which is not very well suited with the current PHP session's interface.

The reason why this handler can not be a complete drop-in replacement for every use-case (like heavily complex data in session for complex ajax application) is, well, in the most part, it's due to the limit of the current PHP session interface and the default behavior.

However, it's perfectly fine if you use this library just for sign-in and sign-out, even without setting the error handler, because awesome google-cloud-php will retry upon failure, so actually failures other than conflict are really rare.

Also it's perfectly fine if you use this library with the error handler installed, if you have complex session data, and you don't want to lose them.

I can't recall ever running into one in the default implementation.

Hmm, I think you're lucky! Even filesystem based simple session handler can fail. In my opinion, if you don't want to lose any session data, only reliable way is to handle errors like I do in the sample, it applies to any other session implementations.

dwsupplee · 2016-10-27T23:13:05Z

The reason why this handler can not be a complete drop-in replacement for every use-case (like heavily complex data in session for complex ajax application) is, well, in the most part, it's due to the limit of the current PHP session interface and the default behavior.

However, it's perfectly fine if you use this library just for sign-in and sign-out, even without setting the error handler, because awesome google-cloud-php will retry upon failure, so actually failures other than conflict are really rare.

I think this right here is what I'm looking for in the docs :). My only real concern at this point is someone thinking they can use it as a drop in replacement and being surprised by issues. If we make it clear what the best use cases are then I feel a lot better about putting it into people's hands.

tmatsuo · 2016-10-27T23:14:44Z

@dwsupplee
Good call. I will try to come up with a good documentation.

tmatsuo · 2016-10-27T23:51:56Z

It is my first attempt for the docblock update! PTAL @dwsupplee @jdpedrie

dwsupplee

The added documentation looks really helpful. Thanks for adding it!

src/Datastore/DatastoreSessionHandler.php

@@ -26,6 +26,31 @@
 *
 * {@see http://php.net/manual/en/class.sessionhandlerinterface.php}
 *
+ * Instead of storing the session data in a local file, it stores the data to
+ * Cloud Datastore. Biggest benefit of doing this is the data can be shared by


src/Datastore/DatastoreSessionHandler.php

+ * The downside of using Cloud Datastore is the write operations will cost you
+ * some money, so it is highly recommended to minimize the write operations
+ * with your session data with this handler. In order to do so, keep the data
+ * in the session as few as possible; for example, it is ok to put only


src/Datastore/DatastoreSessionHandler.php

+ * operations will cause the Datastore write and then it will cost you lot of
+ * money.
+ *
+ * This handler doesn't provide pesimistic lock for session data. Instead, it


src/Datastore/DatastoreSessionHandler.php

+ *
+ * If you are building an ajax application which may issue multiple requests
+ * to the server, please design the session data carefully, in order to avoid
+ * possible data contentions. Also please see the 2nd exmaple in this docbock


tmatsuo · 2016-10-28T21:25:15Z

@jdpedrie Thanks for the review, all done.

src/Datastore/DatastoreSessionHandler.php

 *
 * If you are building an ajax application which may issue multiple requests
 * to the server, please design the session data carefully, in order to avoid
- * possible data contentions. Also please see the 2nd exmaple in this docbock
- * for how to properly handle errors on `write` operations.
+ * possible data contentions. Also please see the 2nd exmaple below for how to


src/Datastore/DatastoreSessionHandler.php

+ *
+ * Example without error handling:
+ * ```
+ * use Google\Cloud\Datastore\DatastoreClient;


src/Datastore/DatastoreSessionHandler.php

+ * session_save_path('sessions');
+ * session_start();
+ *
+ * # Then read and write the $_SESSION array.


src/Datastore/DatastoreSessionHandler.php

+
+    /**
+     * @param DatastoreClient $datastore Datastore client.
+     * @param int [optional] $gcLimit A number of entities to delete in the


src/Datastore/DatastoreSessionHandler.php

+    }
+
+    /**
+     * @param string $savePath The value of `session.save_path` setting will be


src/Datastore/DatastoreSessionHandler.php

+        return true;
+    }
+
+    public function close()


jdpedrie · 2016-10-31T14:14:52Z

code looks good to me, but there are some docs issues still to be addressed. :)

Thanks for working through these issues with us, @tmatsuo.

jdpedrie · 2016-10-31T14:15:49Z

You can test whether the parser fails by running this command:

$ composer google-cloud docs

tmatsuo · 2016-10-31T18:56:21Z

@jdpedrie Alright, now it seems like it compiles, PTAL

src/Datastore/DatastoreSessionHandler.php

+ *
+ * $cloud = new ServiceBuilder();
+ * $datastore = $cloud->datastore();
+ * // or just $datastore = new DatastoreClient();


src/Datastore/DatastoreSessionHandler.php

+     *        used here. It will use this value as the Datastore kind.
+     * @return bool
+     */
+    public function open($savePath, $sessionName)


tmatsuo · 2016-10-31T19:18:09Z

@jdpedrie @bshaffer PTAL

src/Datastore/DatastoreSessionHandler.php

+/**
+ * Custom session handler backed by Cloud Datastore.
+ *
+ * {@see http://php.net/manual/en/class.sessionhandlerinterface.php}


jdpedrie · 2016-10-31T20:31:00Z

Looks good! Thanks @tmatsuo

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 21, 2016

Adding a custom session handler with Cloud Datastore.

ab60a5a

tmatsuo force-pushed the datastore-session branch from 85c7aec to ab60a5a Compare October 21, 2016 05:07

jdpedrie reviewed Oct 21, 2016

View reviewed changes

jdpedrie mentioned this pull request Oct 21, 2016

Allow per-query overrride of the datastore namespaceId #214

Merged

Added a constructor

f829d12

Unit test

f63fe21

tmatsuo changed the title ~~[WIP] Adding a custom session handler with Cloud Datastore.~~ Adding a custom session handler with Cloud Datastore. Oct 22, 2016

Use $sessionName for the kind

f91c494

Takashi Matsuo added 2 commits October 25, 2016 14:40

Fixed timing bug in the test

d7bfb7f

Fixed more timing bug in the tests

4e1a99e

dwsupplee reviewed Oct 25, 2016

View reviewed changes

src/Datastore/DatastoreSessionHandler.php

sprintf('Datastore lookup failed: %s', $e->getMessage()),

E_USER_NOTICE

);

return '';

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Takashi Matsuo added 2 commits October 26, 2016 11:45

Use session.save_path for namespace.

76fa9b4

Use transaction

a6a64b9

jdpedrie reviewed Oct 26, 2016

View reviewed changes

Throw \InvalidArgumentException instead of replacing silently.

8da61b7

Added prefered use case in the docblock

f105389

dwsupplee suggested changes Oct 28, 2016

View reviewed changes

Doc tweak

43761ad

dwsupplee reviewed Oct 28, 2016

View reviewed changes

TYPO

9aaaa55

dwsupplee approved these changes Oct 28, 2016

View reviewed changes

jdpedrie suggested changes Oct 31, 2016

View reviewed changes

Takashi Matsuo added 2 commits October 31, 2016 11:51

Docs tweak

d3cb822

Use ServiceBuilder

ca10b4e

bshaffer requested changes Oct 31, 2016

View reviewed changes

code review

e37da9d

jdpedrie reviewed Oct 31, 2016

View reviewed changes

src/Datastore/DatastoreSessionHandler.php

/**

* Custom session handler backed by Cloud Datastore.

*

* {@see http://php.net/manual/en/class.sessionhandlerinterface.php}

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Fixed the @see tags

021c628

jdpedrie approved these changes Oct 31, 2016

View reviewed changes

bshaffer approved these changes Nov 1, 2016

View reviewed changes

dwsupplee merged commit c71c796 into googleapis:master Nov 1, 2016

jdpedrie mentioned this pull request Nov 10, 2016

Prepare v0.12.0 #238

Merged

tmatsuo deleted the datastore-session branch April 24, 2017 22:55

Adding a custom session handler with Cloud Datastore. #213

Adding a custom session handler with Cloud Datastore. #213

Conversation

tmatsuo commented Oct 21, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tmatsuo commented Oct 21, 2016

tmatsuo commented Oct 22, 2016

tmatsuo commented Oct 25, 2016

tmatsuo commented Oct 25, 2016

dwsupplee commented Oct 25, 2016

This comment was marked as spam.

This comment was marked as spam.

tmatsuo commented Oct 25, 2016

tmatsuo commented Oct 25, 2016

jdpedrie commented Oct 26, 2016

jdpedrie commented Oct 26, 2016

tmatsuo commented Oct 26, 2016 • edited Loading

dwsupplee commented Oct 26, 2016

tmatsuo commented Oct 26, 2016

tmatsuo commented Oct 26, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

dwsupplee commented Oct 26, 2016

jdpedrie commented Oct 27, 2016

tmatsuo commented Oct 27, 2016

tmatsuo commented Oct 27, 2016 • edited Loading

dwsupplee commented Oct 27, 2016

tmatsuo commented Oct 27, 2016

tmatsuo commented Oct 27, 2016 • edited Loading

dwsupplee left a comment • edited Loading

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tmatsuo commented Oct 28, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

jdpedrie commented Oct 31, 2016

jdpedrie commented Oct 31, 2016

tmatsuo commented Oct 31, 2016

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

tmatsuo commented Oct 31, 2016

This comment was marked as spam.

This comment was marked as spam.

jdpedrie commented Oct 31, 2016

tmatsuo commented Oct 26, 2016 •

edited

Loading

tmatsuo commented Oct 27, 2016 •

edited

Loading

tmatsuo commented Oct 27, 2016 •

edited

Loading

dwsupplee left a comment •

edited

Loading