BUG: AsyncDocumentSession.Store() not assigning DocumentId

[James Hancock]

If you use a synchronous session and call .Store() it will assign the Id to the item without issue and you'll have access to it, and if the document has a property for it, it will show up there automatically.

If however you do the same thing with an async session, the Id will not be set. This means that you cannot create a second document that has a reference to the first because you can't get the Id of the first until you SaveChanges().  This becomes majorly problematic when you need to not persist these to the document store until a later time.

Here's a reproducing case.

        [TestMethod]

        public void TestIdOnStore()

        {

            using (var store = NewDocumentStore())

            {

                using (var session = store.OpenAsyncSession())

                {

                    var account = new TestAccount(TestAccountTypes.BankAccount, "Test", "123");

                    session.Store(account);

                    System.Diagnostics.Debug.Assert(!string.IsNullOrWhiteSpace(session.Advanced.GetDocumentId(account)));

                }

            }

        }

Doesn't matter what TastAccount is, it can be any class at all with any number of properties, it will still assert.

[Chris Marisic]

I believe this is by design.

[Oren Eini (Ayende Rahien)]

This isn't a bug, it is by design. See: http://issues.hibernatingrhinos.com/issue/RavenDB-834

--
You received this message because you are subscribed to the Google Groups "ravendb" group.
To unsubscribe from this group and stop receiving emails from it, send an email to ravendb+u...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[James Hancock]

Is there any progress on fixing this? The way it is right now makes asyncsessions very limited in their use and given the power of async in .NET 4.5 and especially scalability with mvc.net 4.5 and web api, this is a pretty big deal!

 

James Hancock

 

[Chris Marisic]

You say fixing as if something was broken.

For the model you want, it already exists, just create the object and save changes immediately.

[James Hancock]

As stated this can’t happen. I have to create the object with everything in it, then use that object to send to a 3rd party, and then and only then once a success comes back from the 3rd party, can I save it.

 

Further, manually assigning the ID doesn’t include the shard information on the Id and if you manually put the shard information on the ID, when you save, it adds the shard information a second time.

 

Hence there is no work around for the lack of StoreAsync() on the AsyncDocumentSession, and because it behaves completely differently than the sync version, this should be considered a bug.

 

James Hancock

 

From: Chris Marisic
Sent: ‎February‎ ‎5‎, ‎2013 ‎12‎:‎14‎ ‎PM
To: rav...@googlegroups.com
Subject: Re: [RavenDB] BUG: AsyncDocumentSession.Store() not assigning DocumentId

 

[Chris Marisic]

I don't view this as a bug.

StoreAsync(foo)
StoreAsync(foo1)
StoreAsync(foo2)
StoreAsync(foo3)
...

SaveChanges()

I sure wouldn't want this to pound my server with like 50 requests

[James Hancock]

Store(foo)

 

does the same thing and returns the Id, so why would StoreAsync behave any differently?

 

I realize that Async was originally designed for Silverlight, but the reality is that it’s more likely going to be used for .NET 4.5 and mvc or web.api now because it massively increases the scalability of the mvc.net or web.api server.

 

There is no reason to expect that this will be done remotely, and if it’s fine for .Store() on a synchronous session to do it, then it should be fine for the Async version to do the same asynchronously.

 

Thus if you’re going to allow pounding of the server one way, then there’s no logical reason why it shouldn’t allow pounding of the server the other way. And given that there is no way that you can do this manually and actually get the real id with sharding information, this is a blocking issue for implementation of any non-trivial system.

 

James Hancock

 

From: Chris Marisic
Sent: ‎February‎ ‎5‎, ‎2013 ‎12‎:‎34‎ ‎PM
To: rav...@googlegroups.com
Subject: Re: [RavenDB] BUG: AsyncDocumentSession.Store() not assigning DocumentId

 

[Chris Marisic]

I don't think this is an accurate statement. With a synchronous session it uses hi-lo and works with blocks of ranges. With async that doesn't really align with hi-lo, it would request a new hi value full with the intention of using a single lo value. Sure it could work, but it would hammer the server for every store request which is exactly why it doesn't behave like that.

[James Hancock]

Why can’t Async do hi/lo exactly the same way?

 

Or alternatively, can I assign my own and actually make it get everything including the sharing info?

 

James Hancock

 

From: Chris Marisic
Sent: ‎February‎ ‎5‎, ‎2013 ‎12‎:‎43‎ ‎PM
To: rav...@googlegroups.com
Subject: Re: [RavenDB] BUG: AsyncDocumentSession.Store() not assigning DocumentId

 

[Oren Eini (Ayende Rahien)]

James,

It can be made to work the same way. We are waiting for a pull request from someone who can do that.

We are currently busy elsewhere.

[Felipe Leusin]

I`m going to dry to do this week.

I`ve got something working but traveled last week and couldn`t get to it.

Imho, it should have StoreAsync() that behaves exactly like the Store() when regarding to generating Ids and we should drop Store() from AsyncDocumentSession. What do you guys think?

[Khalid Abuhakmeh]

That makes sense Felipe, but it might be a good idea to just deprecate (comments and attributes) the Store method for now and then remove it entirely later.

[Chris Marisic]

If it issues more requests to satisfy this than session.Store, i would not be interested in that code path. I find it entirely reasonable for ids to be generated on SaveChanges for async.

On Tuesday, February 5, 2013 6:57:19 PM UTC-5, Felipe Leusin wrote:

[Kijana Woodard]

These two threads have been...interesting. I'll chime in on both threads here. (Warning: written in haste.)

I understand why async session waits until save changes. I also see why being symmetric behavior wise to sync session might be a good thing. I also see why "send a pull request, this is low priority" is a valid response.

How's that for ambivalence? :-)

Assume you just make Store set the id in a sync manner. What we're really talking about is, every N store requests, the session will reach out to the db for a new hi/lo range. Considering you can also tweak the size of that range, this seems like a micro optimization.

That said, fine, we will await the 5 ms to get the hi/lo. But why StoreAsync? 

Store on the async session can just await getting the new hi/lo range itself. 

IMO, all of the XyzAsync methods feel like api clutter. If I'm using the async session, I would expect that the relevant methods (ToList/Count/Store/SaveChanges) return Tasks that I can choose to await rather than having to call the XyzAsync equivalent method.

Not having looked at the Async Session code, it may be impossible to redefine the return signatures of the various methods on the interface without a lot of work throughout the code base. If so, then XyzAsync is a livable compromise. Still, async session Store can just handle do its own "awaiting". Await would only be necessary assuming hi/lo is in play for the Store. StoreAsync on a doc with an id set to "config/staticname" is strange. Having to remember to use the right one is annoying.

@James - I'm not convinced you are as belligerent as you appear in your thread posts. However, from the perspective of people sitting in their own homes or offices that have their own problems to contend with, your tone doesn't help solicit thoughtful responses. Most folks won't have an accounting background, so simply saying that the domain is _easy_ isn't enough.

As to stale indexes not being "safe by design", this is standard eventual consistency. To Chris' point, Raven gives you choices and levers. In a standard rdbms setup, you have no choice but to be immediately consistent. So you or the dba spends time trying to compromise the design for reads _and_ writes. 

In Raven, you can explicitly choose that causal observers of an account can see stale data. This is perfectly fine when people are trying to "get an idea about the state of the account". Oh I've got  ~$20k in that account. It doesn't matter that there a $7 check was clearing at around the time I queried.

For users/processes that are making _decisions_ based on the current state of the account, you can explicitly choose that _they_ see the latest up to date information through Load or making them wait on the indexes to complete. This is always imperfect because a human could be in the process of making a decision that will affect the state in the near future (seconds) but the system has been made aware of this fact. Your software simply records actions AsOf time and can remain logically consistent even if one could surmise that the user _might have_ acted differently with newer information.

In the end, you could make Raven behave exactly like an RDBMS and make every user wait for everything to be consistent. This is a choice, and the fact that it is a choice is important, and powerful.

In terms of modeling, there are dozens of ways to model any domain. Anyone commenting here is surmising based on limited knowledge. Likely their entire understanding comes from the paragraphs you have written. Often this will not connect with your mental model because you have so much more information baked in.

I actually do consult/contract for a living. It sounds like you have an interesting problem space regardless of the persistence technology you choose. My rate is considerably cheaper than Chris' proposal. I have availability May 1.

[Oren Eini (Ayende Rahien)]

Chris,

It would work the same way as Store would. Hilo with a single request per batch, including auto sizing of the batches.

[Chris Marisic]

If there's no technical/feasibility issues to achieve that, it seems like it would be a good enhancement then.

[Oren Eini (Ayende Rahien)]

inline

On Wed, Feb 6, 2013 at 5:19 PM, Kijana Woodard <kijana....@gmail.com> wrote:

These two threads have been...interesting. I'll chime in on both threads here. (Warning: written in haste.)

I understand why async session waits until save changes. I also see why being symmetric behavior wise to sync session might be a good thing. I also see why "send a pull request, this is low priority" is a valid response.

How's that for ambivalence? :-)

Keep this up, and I'll sign you up to a political party chosen at random, and no one will be able to tell the difference :-)

 

Assume you just make Store set the id in a sync manner. What we're really talking about is, every N store requests, the session will reach out to the db for a new hi/lo range. Considering you can also tweak the size of that range, this seems like a micro optimization.

Actually, we auto tune that value ourselves, you don't need to worry about it.

 

That said, fine, we will await the 5 ms to get the hi/lo. But why StoreAsync? 

Store on the async session can just await getting the new hi/lo range itself. 

That requires you to use do async, and the Store method is currently not doing that.

We can't just wait, because in WinRT & Silverlight, there _is_ no way to wait for async stuff.

It would have to be async and that requires changing the signature. I _really_ want to avoid having both Store and StoreAsync in there ,too.

 

IMO, all of the XyzAsync methods feel like api clutter. If I'm using the async session, I would expect that the relevant methods (ToList/Count/Store/SaveChanges) return Tasks that I can choose to await rather than having to call the XyzAsync equivalent method.

We are following the .NET naming conventions here for async API.

 

Not having looked at the Async Session code, it may be impossible to redefine the return signatures of the various methods on the interface without a lot of work throughout the code base.

It is actually a different interface, but mostly I want to avoid people thinking that they can do things like:

session.Store(foo);

session.SaveChanges();

And NOTHING happens because the Store kicked off an async process that didn't complete by the time SaveChanges was called.

We want it to be clear that you need to await of that.

 

If so, then XyzAsync is a livable compromise. Still, async session Store can just handle do its own "awaiting".

No, it can't, see above.

[Oren Eini (Ayende Rahien)]

There isn't any real problem in doing so. It is just a matter of resource allocation more than anything else.

[Felipe Leusin]

So far this was quite easy to implement, going to have to test with Sharding and other situations before a PullRequest.

Just one thing, want me to leave the removing of Store from IDocumentSession to you guys? I reckon this will be a big breaking change and might be better to just deprecate and remove in Raven 2.1 or something.

[Oren Eini (Ayende Rahien)]

How about we do it like this?

Move the Store to the Advanced (with a better name)

Create an extension method on IAsyncDocumentSession and mark _that_ as deprecated?

[Chris Marisic]

If you're going to go that route, you could actually just obsolete the method and make people update the things to either use advanced or storeasync

[Oren Eini (Ayende Rahien)]

I want to avoid just having that big a breaking change.

[Chris Marisic]

Most users will ignore deprecation warnings until it becomes obsoleted anyway. I'm always fine with build failures, what i can never tolerate is nonobvious runtime differences.

[Oren Eini (Ayende Rahien)]

When I am talking about deprecating something, I am thinking about

[Obselete("Use other method")]

[Kijana Woodard]

Hmmmm. I think I understand. You can do async sessions on winrt, etc, but you can't await?

That sucks.

[Oren Eini (Ayende Rahien)]

You can do await, yes.

But you cannot _wait_.

task.Result in Silverlight (and I think WinRT) == deadlock

[Oren Eini (Ayende Rahien)]

If the task isn't done yet

[Felipe Leusin]

Then why not just mark the Store as Obsolete and remove it on 2.1, 3.0 or something?

I fail to see a point in moving it to Advance and creating an extension method (unless this is some sort of pratice inside RavenDB for deprecation), the only pratical difference between Store and StoreAsync is that StoreAsync uses the asyncDocumentKeyGenerator on certain cases before calling the same StoreInternal() but always passing the id.

I think for now we can just mark Store as Obsolete("You should call StoreAsync if you wish to obtain an Id") and when time comes, remove it from IAsyncDocumentSession.

[Oren Eini (Ayende Rahien)]

I want to avoid having it in the intellisense, even as an obselete method.

This way adding back the support is just adding a namespace.

[Felipe Leusin]

Ok,

I'll send the PR this weekend then.

[Felipe Leusin]

What should the namespace be? I couldn't find one in the project.

[Oren Eini (Ayende Rahien)]

namesapce Raven.Client.Deprecated ? 

[Matt Johnson]

One point of note - this approach for deprecation works fine for the compiler, but not at the CLR level.  Extension methods are just syntactic sugar.

So if the user was to update their raven client and recompile - no problem.  But if they just changed out the dll, it wouldn't work.

I don't think that's very likely - but just thought I would mention it.

[Oren Eini (Ayende Rahien)]

We don't support just upgrading the dll.

[Felipe Leusin]

I actually included in Raven.Client.Extensions because it fit the motto of the DatabaseCommandsExtensions

///<summary>

/// Extension methods that make certain database command operations more convenient to use

///</summary>

But I can create Raven.Client.Deprecated if you prefer. Another thing, want me to update the tests that current run asyncSession.Store to StoreAsync()?

[Oren Eini (Ayende Rahien)]

Doesn't really matter.

And I don't want to update those tests, they are working, and should keep working :-)