Situation where Fiber.current does not return the current fiber #177

Closed
yortus opened this Issue Jul 2, 2014 · 5 comments

Comments

Projects
None yet
3 participants

yortus commented Jul 2, 2014

Here is a repro to demonstrate this issue.

var Fiber = require('fibers');
var Fiber2 = require('fibers2'); // another copy elsewhere in node_modules

var f = Fiber(function () {
    console.log((Fiber.current || {}).id); // returns 777
    console.log((Fiber2.current || {}).id); // returns undefined
});

f.id = 777;
f.run();

To run this, make a copy of the 'node_modules/fibers' directory at 'node_modules/fibers2'.

It may look contrived, but it's a simple model of a situation where the fibers module appears more than once in a module dependency graph and isn't (or can't be) deduped.

Since a node process has one current execution context, I would have thought that all instances of the fibers module loaded into the process should return the same value of Fiber.current at a particular point in time. i.e., Fiber2.current in the repro above is erroneous to return undefined, because the current execution context is in a fiber when the call is made.

EDIT: simplified description.

yortus commented Jul 3, 2014

BTW I've got the above behaviour on:

  • Windows 7 x64 / Node 0.10.25
  • Ubuntu 12.04 / Node 0.10.26
Owner

laverdet commented Dec 18, 2014

Would something like this fix it?

diff --git a/fibers.js b/fibers.js
index 2d80c84..6fade1a 100644
--- a/fibers.js
+++ b/fibers.js
@@ -1,3 +1,6 @@
+if (process.fiberLib) {
+   return module.exports = process.fiberLib;
+}
 var fs = require('fs'), path = require('path');

 // Seed random numbers [gh-82]
@@ -14,4 +17,4 @@ try {
 }

 // Pull in fibers implementation
-module.exports = require(modPath).Fiber;
+process.fiberLib = module.exports = require(modPath).Fiber;

yortus commented Dec 21, 2014

@laverdet that's more or less exactly what I'm doing in asyncawait to guard against this problem and it works great. However if other libraries in the module tree (ie apart from asyncawait) were also using fibers, the problem could still appear. Moving the check down into the fibers source, as per your code above, would be much more reliable.

@laverdet laverdet added a commit that referenced this issue Jan 3, 2015

@laverdet laverdet Guard aginst multiple binary includes
npm can install fibers more than once which tends to lead to trouble.

Fixes #102, #177
d9bc3a7

laverdet closed this Jan 3, 2015

Animadei commented Jan 5, 2015

Was tearing out my remaining strands of hair over the Christmas break trying to figure out why our scripts faced so many different crashes, aborts, and exceptions. The slightest timing change caused a different but highly repeatable error. The undefined behaviors exhibited on Raspberry Pi over the latest Arch Linux updates but not on Windows 8.1. Went as far as getting node.js v0.11 to compile and run on ARM v6 and mentally prepared myself to use GDB on node and fibers. While looking through every comment available on the planet about this issue, I perused at the most recent Fibers commits. What was this recent change list from 3 days ago? I upgraded to Fibers 1.0.4 with low expectations. The scripts kept running for thousands of iterations rather than the usual 1 to 35 iterations before crapping out. These strange errors disappeared. Not one, but all. Thanks to both of you I may live another day without touching gdb.

Owner

laverdet commented Jan 5, 2015

@Animadei I think the commit that fixed your problem was probably 3e15494 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment