search

wiredtiger / node-wiredtiger

Other
12 stars 4 forks source link

Wrap WT_ASYNC_CALLBACK rather that add an app_data field. #1

Closed agorrod closed 10 years ago

agorrod commented 10 years ago

@sueloverso Could you take a look at this change for me please? This is in the Node wrapper, I'd originally modified WiredTiger so that the WT_ASYNC_OP structure has an app_data field. That code was working.

This change attempts to wrap WT_ASYNC_CALLBACK instead, as you did with ex_async.c, somehow it's not working. My Node application ends up hanging - which means that the callbacks aren't being made from WiredTiger.

Could you look through and see if you can spot my mistake? It's Friday afternoon - so hopefully it's something stupid.

agorrod commented 10 years ago

I thought maybe the issue was that I needed to wrap the structure in extern "C", since this is C++ code. I tried that and it didn't help.

agorrod commented 10 years ago

@sueloverso I've figured this out. I was using a single global instance of my callback wrapper, and updating the cookie pointer for each call. So when the callback came in it was usually getting the wrong pointer. My code is different to the example in ex_async.c - you want a single bit of context that is shared by all callbacks - I want a different context for each callback.

I can solve this by allocating a new callback object for each async operation, and freeing it after the operation has completed. That's not very intuitive for me though - it makes the code complex, and it's hard to know when the memory for the callback object can be free'd (e.g: Will WiredTiger ever reference it again after it's made the callback?)

What I really want is somewhere in the WT_ASYNC_OP to stash something, rather than in the callback handle. Would you consider adding a field for user data to the WT_ASYNC_OP handle?

@michaelcahill I'm cc'ing you because I'm proposing an API change.

michaelcahill commented 10 years ago

My 2c: I think the fact that wtperf uses op->c.lang_private is a sign that having an exported field in WT_ASYNC_OP makes sense.

sueloverso commented 10 years ago

@agorrod I pushed a change to expose the private field without showing the embedded cursor structure of the op handle. If you merge up to develop (I notice this branch is off master instead), you can get to it via op->async_app_private. Let me know if this does not need your needs, or if you feel we need two private fields, one for languages and another separate one for the app.

agorrod commented 10 years ago

This has been fixed - it will be in the next WiredTiger release.