Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal of Polling~Queue: Sync Hook Problem #249

Closed
wentout opened this issue Oct 22, 2018 · 4 comments
Closed

Proposal of Polling~Queue: Sync Hook Problem #249

wentout opened this issue Oct 22, 2018 · 4 comments

Comments

@wentout
Copy link

wentout commented Oct 22, 2018

Hi!

First of all I wish to say many thanks for all this happens!
Really nice API, and works just good enough!

And, I'm very sorry, but I need to ask a question about the issue I'm under and unfortunately unable to solve by myself. Despite everything with Async contexts seems to be working good, there is a problem with Sync instead.

If you can remember @trevnorris described an Idea about Continuation Local Storage: Here Exactly.

This is my dream too. And, I'm very sorry, but I'm unable to get rid of this CLS implementation Idea.
So, the basis of this sync issue relies here: UserModeQueuing.

And, as for me -- Continuation Local Storage is an Idea about to pass the logic context, some sort of ability to way through all the async operations. But, as I see, the problem is not with the Async anymore, it is on the other side, so let me please show you:

const queueArray = [];

setInterval(() => {
	queueArray.forEach(task => {
		task();
	});
}, 1000);

const FunctionWhereTrackingStarts = () => {
	// Here we are still in the same logi
	// And now we create function
	// And push it to queue
	queueArray.push(() => {
		// **HERE** we have no ability
		// to reach the same logical context
		// because it executes in the other scope
	});
};

As far as I can see and track the code, there is a way to solve this. And, again, with Async Hooks.
That anonymous function, which is pushed to queueArray. I see this pattern as a "context split". It receives it's own context [[Scope]] in runtime. I can easily track it via Inspector or ndb. So, when the hook is running, between "before" and "after" -- it is synchronous code. And if there will be a way to track New Function Scope creation via Async Hooks, there will be a way to receive new context for that function: new AsyncID and the same TriggerId. This must be somwhere inside of Isolate.cc of V8, or somewhere nearby.

I understand tracking graph will split in two ways, like branch in git.
But for that polling issue, it is the only way to solve it, and it will work, because it works good with patching. And, actually, everybody who has a sort of working implementation of CLS (npm: cls-hooked, continuation-local-storage) did some implementation of passing through (me too). But it requires Direct Patching of underlying libraries queueing~poolling methods.

Please, for the best example of patching look into cloud-trace especially here. It is the toolset of patching most used NodeJS libs with the way to make CLS working. It is a really hard effort done there and much the best way right now.

But if there will be the way to implement hooks for "splitting/forking" of the scope/context itself -- it will be much easier way to track and to "jump through" all of this "underlying" code.

And also I think that beautifull code will win too node-clinic-bubbleprof.

And, also, as I can see there is no way to break something with JS behaviour itself, if this implementation will be done. And off the opposite there is no way to create unused New Function Context in the sync road between "before" and "after". And even if on that road there will be some sync callback interception, it is ok, it is on the same road. And there is also no way to occasionally make that new context creation interception for function, that is ouside of running context. Even if that function is passed as a callback, that means this function is "under" and "withing the same" Logical Context we need to track for CLS.

And I'm sorry for interrupting you with all this. But I'm not yet C~C++ developer. I'm unable to make it by myself. I just use node in production, and monkey patch all underlying code that utilise async_hooks. And I don't like the way I do it, because I still hope for a better way.

Seems it also might help here : The problems from my 1+ year experience with CLS · Issue #59 · othiym23/node-continuation-local-storage
othiym23/node-continuation-local-storage#59

@othiym23, @ofrobots, @iliakan please review this proposal

Thank You very much for reading!

And thank You again for everything you already did and continue doing.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented Oct 22, 2018

This is the solution we are expecting third-party modules to implement.

const { AsyncResource } = require('async_hooks');
const queueArray = [];

setInterval(() => {
	queueArray.forEach(task => {
		task();
	});
}, 1000);

const FunctionWhereTrackingStarts = () => {
	// Here we are still in the same logi
	// And now we create function
	// And push it to queue
        const resource = new AsyncResource('SpecialQueue', { requireManualDestroy: true });
	queueArray.push(() => {
                resource.emitBefore();
		// **HERE** we have no ability
		// to reach the same logical context
		// because it executes in the other scope
                resource.emitAfter();
                resource.emitDestroy();
	});
};

@wentout
Copy link
Author

wentout commented Oct 22, 2018

Andreas!
Everywone!

Thank You for the answer!

I Understand, and it is obvious for me, cause I already did the same.

But there is the way how to do this automatically!
And somewone with C++ in touch are able to to implement this.

And, meanwile, library developers, they are not implementing this behaviour.
They do their own, without AsyncResource.
For example, look here
https://github.com/googleapis/cloud-trace-nodejs/blob/master/src/plugins/plugin-mongodb-core.ts
And here
https://github.com/googleapis/cloud-trace-nodejs/blob/e73af2b1eda38faeda50c83f815847d163cdb369/src/trace-api.ts

They are just skipping frames, without any AsyncResource wrapper inside.
So, nobody can actually receive the additional hooks.

And, anyway, it is not a simple solution.
Much simplier will be another hook on sync scope context creation of the

queueArray.push(() => {

I mean not on the push, but on the place where function receives its scope in runtime.
Exactly in place where this pushed function receives it's own scope.

So there will be the following use case:

  • Init Hook : function receives runtime scope [[Scope]]
  • Before Hook : function is called by the pooling or queue
  • After Hook : sync run of the function is finished
  • Destroy Hook : the same as GC does for other hooks

And we are able to help them to get rid of any patching!

And I need this too, I utilise mongoose, and do monkey patching~wrapping... it seems weird for me.

Kind Regards and Best Wishes!

@wentout
Copy link
Author

wentout commented Oct 23, 2018

Also, I think it is necessary to link to this topic:

Formalizing Asynchronous Context - Mike Kaufman & Mark Marron, Microsoft
Here is a link to slides. There is an Idea of Continuation.

And, digging through, may be issues here, or something surrounding I've found that link

I understand, that this Context Creation Hook for [[Scope]] initialisation might dramatically change everything, seems it will be not necessary to track all other hooks anymore.

But also might be a lot of work to be done to implement this hook.

@wentout
Copy link
Author

wentout commented Oct 23, 2018

Finally, I close the issue, as irrelevant to current conditions.
Hope I will have a link to it in future.
Obviously, if it will make sence, there must be a button to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants