Skip to content

Commit

Permalink
The end 💀
Browse files Browse the repository at this point in the history
  • Loading branch information
@laverdet
laverdet committed Apr 13, 2021
1 parent e2a0ed9 commit 8f28098
Showing 1 changed file with 5 additions and 0 deletions.
5 changes: 5 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ abruptly stop working and no one will be able to do anything about it.
I'd like to say thank you to all the users of fibers, your support over the years has meant a lot to
me.

**Update** *[April 13th, 2021]* -- Fibers is not compatible with nodejs v16.0.0 or later. Unfortunately, v8
commit [dacc2fee0f](https://github.com/v8/v8/commit/dacc2fee0f815823782a7e432c79c2a7767a4765) is a breaking
change and workarounds are non-trivial.


fibers(1) -- Fiber support for v8 and Node
==========================================
[![npm version](https://badgen.now.sh/npm/v/fibers)](https://www.npmjs.com/package/fibers)
Expand Down

18 comments on commit 8f28098

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @benjamn @bjouhier @christian-bromann @nex3 @tcf909

I know yall have infrastructure built on this so I wanted to give you an early heads up. I did manage to get it working using the CORO_PTHREAD backend but that's no way to live. nodejs/node@1468c9f also deprecates the undocumented async_wrap API.

@christian-bromann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laverdet thanks for pinging. I knew this day would come 😢. I wonder if there is any chance we can continue support for it. The WebdriverIO project just started an OpenCollective which could be a way to financially support the support of fibers. I am that others have a big interest in this too. I think I already emphasized in the past that fibers is a crucial component for the simplicity of WebdriverIO.

With the change in Chromium do you see any other way to have the functionality of Fibers supported or is it just technically not feasible?

I did manage to get it working using the CORO_PTHREAD backend but that's no way to live.

Can you elaborate on this?

@bjouhier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @laverdet. Thanks for the heads-up. That's very unfortunate because fibers allowed us to have very simple code with all functions of the same color and almost no 'await' noise. I had wished to see some momentum around stackful coroutines but it did not happen 😞.

We'll have to convert our code. I assume that the CORO_PTHREAD fallback has a significant impact on performance.

Thanks so much for all the effort and magic you have put into this project, and for the awesome collaboration/support. I'll miss it!

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the change in Chromium do you see any other way to have the functionality of Fibers supported or is it just technically not feasible?

It'd be trivial if you want to ship a custom build of nodejs to customers. In that case you could revert this change, or even better just publicize the internal v8 data.

Can you elaborate on this?

libcoro supports a bunch of different techniques for creating fibers: https://github.com/laverdet/node-fibers/blob/master/src/libcoro/coro.h#L91-L156. CORO_PTHREAD uses threads instead of coroutines. The API remains the same but fibers are no longer lightweight and cheap. Also this backend doesn't support Windows but adding support wouldn't be tough if you wanted to.

@christian-bromann
Copy link

@christian-bromann christian-bromann commented on 8f28098 Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laverdet just had a chat with @ulan, he mentioned:

If I understand correctly, the Fibers package needs a mapping from v8::Isolate to thread_id. Would it be possible to maintain that mapping on the Node/Fibers side? E.g. by generating the thread_id (which is a simple counter) and inserting/deleting the isolate/thread_id pair on isolate creation and disposal.

Would that be possible?

@laverdet
Copy link
Owner Author

@laverdet laverdet commented on 8f28098 Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easy enough to read v8's thread id but we need to be able to write it. In fact node-fibers already uses hardcoded dereferences to read the thread_id out of internal v8 storage. The layout of PerIsolateThreadData is simple and hasn't changed since it was created so this is fine:
https://github.com/v8/v8/blob/7cc6127b6a8d06c742a0e8d5f507ef7af5452337/src/execution/isolate.h#L530-L534

Fibers reads this internal data structure to guide the rest of the TLS search:
https://github.com/laverdet/node-fibers/blob/master/src/coroutine.cc#L112-L118

Zooming out a little bit you need to convince v8 to "archive" a thread when switching to a new a fiber.
https://github.com/v8/v8/blob/7cc6127b6a8d06c742a0e8d5f507ef7af5452337/src/execution/v8threads.cc#L242-L256
https://github.com/v8/v8/blob/7cc6127b6a8d06c742a0e8d5f507ef7af5452337/src/execution/v8threads.cc#L94-L109

v8 won't archive a thread unless it detects the current thread id has changed. That used to be stored in a traditional thread local key which you can explore on all platforms by manually walking through the key space, but now it is stored in a C++11 thread_local which is going more platform & compiler specific than the existing TLS stuff.

Another approach would be to muck around with ThreadManager but to get a reference to that you have to dive into Isolate which is significantly more dynamic and complex:
https://github.com/v8/v8/blob/7cc6127b6a8d06c742a0e8d5f507ef7af5452337/src/execution/isolate.h#L1832-L1903
->
https://github.com/v8/v8/blob/7cc6127b6a8d06c742a0e8d5f507ef7af5452337/src/execution/v8threads.h#L94-L99

Edit: And this is only addressing the issue on the v8 side of things. There of course remains the issue of async_hooks / async_wrap in nodejs which will have similar hurdles.

@ulan
Copy link

@ulan ulan commented on 8f28098 Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sorry for breaking this package. I was surprised to learn about the breakage because my commit changes a V8 internal data-structure that is not exposed via V8's public API. The change was motivated by performance: C++ thread_local is at least 2x faster that TLS.

As we are adding more concurrency to V8 there will be more changes in the V8 internal thread-related data-structures. Since this package relies on V8 internals rather than the public API, I don't see a quick way to rescue this package unfortunately.

The only solution I see is to propose adding the necessary functions to the public API. I don't guarantee that the V8 team will accept the proposal because we try to minimize exposing of implementation details.

@christian-bromann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ulan thanks for the reply. @laverdet has noted multiple times that using fibers is a bad idea for production code but its value was to big for me (as WebdriverIO maintainer) to not continue to use it. I believe the Node.js ecosystem would appreciate with packages like node-fibers could continue to exist to help abstracting away async/await noise in packages where you constantly run async code served to users that come from the Java world and aren't familiar with async concepts in Node.js.

What would be the best way to make such a proposal?

@ulan
Copy link

@ulan ulan commented on 8f28098 Apr 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a design doc that describes the proposed new API and send it to v8-dev+design@googlegroups.com

https://v8.dev/docs/design-review-guidelines has more details and a link to the design doc template.

Based @laverdet's comment I think that the new API may be too intrusive and the chances that it will be accepted may be slim. (That's my personal opinion without seeing the concrete API proposal.)

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the input Ulan, and apologies for calling you out in the comment above :)

@christian-bromann if you can find someone to contribute a design doc and attempt to push a change upstream through v8 I'd be happy to provide advice. I think what you'd want to do is provide an optional int thread_id parameter on the Locker and Unlocker constructors. It would be zero-cost in terms of runtime and should provide everything that a fibers implementation would need. I anticipate that v8 maintainers would want this feature to live on Isolate::CreateParams but that's a non-starter because the nodejs isolate has already been created by the time the module is loaded.

I agree with Ulan that it seems unlikely that such a change would be picked up, but who knows. It's worth pointing out that Fibers doesn't actually violate any v8 runtime invariants since v8 already supports simultaneous callstacks on concurrent threads. In fact, having a library that's extremely concurrent has been valuable in finding issues that might only manifest during heavy work:

Issue: Provide a correct debugging state in multithreaded isolates
Fix: [debug] Fully implement Debug::ArchiveDebug and Debug::RestoreDebug.

Issue: Failed CHECK()'s in x64 in objects-inl.h with pthreads
Fix: Runtime_NotifyDeoptimized should search for function activation in all thread stacks

Issue: SetResourceConstraints race condition with multiple threads on x64
Fix: Move configuration of ResourceConstraints to Isolate construction

Though you'd want to have reasonable expectations here. If you start now you might be to get this feature in nodejs v18.x, but nodejs v16.x won't happen because it's an ABI breaking change. And this says nothing of the async_hooks nodejs runtime issue, which would also need to resolved.

@bjouhier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ulan and @laverdet for following up. If there is a way to rescue fibers I'll be on board.

My C++ skills are very rusty but I can try an experiment with the guidance that you gave. I cannot guarantee anything but I'd like to try.
Frankly the perspective of having to introduce async/await keywords in large portions of our code base does not thrill me.

Regarding async_hooks, I greped for it in our project and it seems to be only used by dev tools. Do we have a problem if we run code that does not use it?

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, nodejs keeps track of execution stacks through async & callback contexts. I think this lets it give you more helpful stack traces when you have a Promise that was started from a callback. Fibers throws a wrench into that, and can cause nodejs to crash. The workaround is of course more undocumented APIs:

node-fibers/fibers.js

Lines 32 to 103 in 8f28098

var aw = process.binding('async_wrap');
var getAsyncIdStackSize;
if (aw.asyncIdStackSize instanceof Function) {
getAsyncIdStackSize = aw.asyncIdStackSize;
} else if (aw.constants.kStackLength !== undefined) {
getAsyncIdStackSize = function(kStackLength) {
return function() {
return aw.async_hook_fields[kStackLength];
};
}(aw.constants.kStackLength);
} else {
throw new Error('Couldn\'t figure out how to get async stack size');
}
var popAsyncContext = aw.popAsyncContext || aw.popAsyncIds;
var pushAsyncContext = aw.pushAsyncContext || aw.pushAsyncIds;
if (!popAsyncContext || !pushAsyncContext) {
throw new Error('Push/pop do not exist');
}
var kExecutionAsyncId;
if (aw.constants.kExecutionAsyncId === undefined) {
kExecutionAsyncId = aw.constants.kCurrentAsyncId;
} else {
kExecutionAsyncId = aw.constants.kExecutionAsyncId;
}
var kTriggerAsyncId;
if (aw.constants.kTriggerAsyncId === undefined) {
kTriggerAsyncId = aw.constants.kCurrentTriggerId;
} else {
kTriggerAsyncId = aw.constants.kTriggerAsyncId;
}
var asyncIds = aw.async_id_fields || aw.async_uid_fields;
function getAndClearStack() {
var ii = getAsyncIdStackSize();
var stack = new Array(ii);
for (; ii > 0; --ii) {
var asyncId = asyncIds[kExecutionAsyncId];
stack[ii - 1] = {
asyncId: asyncId,
triggerId: asyncIds[kTriggerAsyncId],
};
popAsyncContext(asyncId);
}
return stack;
}
function restoreStack(stack) {
for (var ii = 0; ii < stack.length; ++ii) {
pushAsyncContext(stack[ii].asyncId, stack[ii].triggerId);
}
}
function wrapFunction(fn) {
return function() {
var stack = getAndClearStack();
try {
return fn.apply(this, arguments);
} finally {
restoreStack(stack);
}
}
}
// Monkey patch methods which may long jump
Fiber.yield = wrapFunction(Fiber.yield);
Fiber.prototype.run = wrapFunction(Fiber.prototype.run);
Fiber.prototype.throwInto = wrapFunction(Fiber.prototype.throwInto);

There's some discussion about what would be needed in nodejs here: #365

@christian-bromann
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input @laverdet and @ulan .. unfortunately I neither have the resources nor the experience to make such a proposal. I created an RFC discussion and the current sentiment seems to be to move forward with async/await. Thanks for all the years of maintenance, it has been an amazing package!

@bjouhier
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I did a quick experiment last week-end: I pulled the latest node code and I reverted the V8 change (v8/v8@dacc2fe). Then I rebuilt node and node-fibers.

This was enough to get our project working with a modified node 16 and with the same performance as before (CORO_ASM mode).
Our project does not use async_wrap so I just turned the warning off by commenting the call to setupAsyncHacks() in fibers.js.

This is not in any ways a viable solution moving forwards but it can be a mitigation plan if you have a strong pressure to move to node 16 and not enough time to complete a conversion to async/await.

@vlasky
Copy link

@vlasky vlasky commented on 8f28098 May 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ulan, I am a member of the global community of Meteor developers. A big chunk of my income comes from SaaS solutions developed in Meteor.

The Meteor framework has been relying on node-fibers since its early days in 2012. This breaking change in V8 is now preventing us from updating to Node.js v16.

Getting around this will require a major changes to Meteor and many packages in its ecosystem - a process that will be lengthy and disruptive.

I kindly urge you and your colleagues to co-operate to find an acceptable solution to rescue fiber support.

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please stop pinging Ulan on this, it isn't his problem. The change in v8 was a totally reasonable one.

@vlasky
Copy link

@vlasky vlasky commented on 8f28098 May 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in any way criticising the change to v8. Ulan's justification based on achieving a performance gain is a strong one.

I want only to explore the possibility of finding a practicable solution that preserves the value added by fibers and avoids/reduces collateral damage to Meteor - whether it be by adding a new public API to access the required V8 internals or by creating a Node.js 16 fork like bjouhier has done.

@laverdet
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I laid out a concrete plan for blessed fibers support in v8 here: 8f28098#commitcomment-49517744. I'm super happy to mentor anyone who wants to take this on, but it's just not a project I can personally take on right now.

Please sign in to comment.