Page MenuHomePhabricator
Differential D11425

Fix module imports in Aphlict server
ClosedPublic
Actions

Authored by epriestley on Jan 18 2015, 11:19 PM.
Tags
None
Referenced Files
F14079546: D11425.diff
Fri, Nov 22, 8:41 AM
Unknown Object (File)
Mon, Nov 18, 3:16 PM
Unknown Object (File)
Thu, Nov 14, 7:16 AM
Unknown Object (File)
Mon, Nov 11, 8:46 AM
Unknown Object (File)
Sun, Nov 10, 2:46 AM
Unknown Object (File)
Tue, Nov 5, 7:40 PM
Unknown Object (File)
Thu, Oct 24, 9:33 PM
Unknown Object (File)
Thu, Oct 24, 9:30 PM
View All Files
Subscribers
epriestley
Korvin

Details

Reviewers
joshuaspence
Maniphest Tasks
T6987: Aphlict server does not respond to http requests on configured ports.
Commits
Restricted Diffusion Commit
rP4636833f3dd8: Fix module imports in Aphlict server
Summary

This was broken in D11383. Basically, I had the ws module installed globally whilst testing, but the changes made do not work if the ws module is installed locally (i.e. in the ./support/aphlict/server/node_modules directory). After poking around, it seems that this is due to the sandboxing that is done by JX.require.

A quick fix is to just not use JX.require, although you may have a better idea?

The error that is occurring is as follows:

<<< UNCAUGHT EXCEPTION! >>>

Error: Cannot find module 'ws'
    at Function.Module._resolveFilename (module.js:338:15)
    at Function.Module._load (module.js:280:25)
    at Module.require (module.js:364:17)
    at require (module.js:380:17)
    at extra.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:48:16)
    at /usr/src/phabricator/support/aphlict/server/lib/AphlictClientServer.js:10:17
    at Script.(anonymous function) [as runInNewContext] (vm.js:41:22)
    at Object.JX.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:58:6)
    at Object.<anonymous> (/usr/src/phabricator/support/aphlict/server/aphlict_server.js:102:4)
    at Module._compile (module.js:456:26)
>>> Server exited!
Test Plan

Now able to start the Aphlict server.

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3899
Build 3911: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

joshuaspence updated this revision to Diff 27447.Jan 18 2015, 11:19 PM
joshuaspence retitled this revision from to Fix module imports in Aphlict server.
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJan 18 2015, 11:19 PM
Herald added subscribers: epriestley, Korvin. · View Herald Transcript
joshuaspence updated this object.Jan 18 2015, 11:20 PM
joshuaspence edited edge metadata.
joshuaspence updated this revision to Diff 27448.Jan 19 2015, 1:36 AM

Change default bindings

joshuaspence updated this revision to Diff 27449.Jan 19 2015, 1:36 AM

Oops, updated wrong diff

joshuaspence added a comment.Jan 19 2015, 10:33 AM

Right, I see the issue now. Basically, we override the require function in the sandbox as follows:

sandbox.require = function(thing) {
  if (thing == 'javelin') {
    return require(dir + '/' + thing);
  } else {
    return require(thing);
  }
};

Where require refers to the require function from the init_node.js file. This maens that in order to properly import the ws module, the path needs to be specified relative to /weboot/rsrc/externals/javelin/core/init_node.js. Alternatively, an absolute path can be specified (i.e. require(__dirname + '/../node_modules/ws').

epriestley commandeered this revision.Jan 19 2015, 7:11 PM
epriestley edited reviewers, added: joshuaspence; removed: epriestley.
epriestley updated this revision to Diff 27459.Jan 19 2015, 7:14 PM

Slightly cleaner fix, I think?

  • Use JX.require() only for Web-facing modules.
  • Use require() for Node-facing modules.
  • Use lots of relative paths, which are gross-ish, but probably cleaner on the balance.
    • I sure don't miss managing includes in libphutil environents.
epriestley added a comment.Jan 19 2015, 7:15 PM

To test this, I ran the server with a local ws, verified my browser connected to it, and then sent myself a test notification.

epriestley mentioned this in T6987: Aphlict server does not respond to http requests on configured ports..Jan 19 2015, 7:18 PM
epriestley added a task: T6987: Aphlict server does not respond to http requests on configured ports..

Fixes T6987.

epriestley updated this revision to Diff 27460.Jan 19 2015, 7:30 PM
  • Very slightly cleaner fix.
epriestley removed a reviewer: Blessed Reviewers.Jan 19 2015, 7:31 PM
joshuaspence accepted this revision.Jan 19 2015, 7:41 PM
joshuaspence edited edge metadata.

Two minor inlines, otherwise this looks good.

support/aphlict/server/aphlict_server.js
10

I think that by require-ing this here, we bypass the whole try { require('ws'); } stuff. Specifically, I would expect this to throw an unhelpful error message if the ws module is missing.

webroot/rsrc/externals/javelin/core/init_node.js
43 ↗(On Diff #27460)

I think it's reasonable to still pass this in.

This revision is now accepted and ready to land.Jan 19 2015, 7:41 PM
epriestley updated this revision to Diff 27463.Jan 19 2015, 7:45 PM
epriestley edited edge metadata.
  • Do library includes later to pick up 'ws' issue.
  • Continue providing a corrected __dirname in Javelin for consistency.
Closed by commit rP4636833f3dd8: Fix module imports in Aphlict server (authored by epriestley, committed by epriestley). · Explain WhyJan 19 2015, 7:46 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
support/
aphlict/
server/
6 lines
lib/
2 lines
2 lines

Diff 27448

View Options

support/aphlict/server/aphlict_server.js

Loading...
View Options

support/aphlict/server/lib/AphlictAdminServer.js

Loading...
View Options

support/aphlict/server/lib/AphlictClientServer.js

Loading...
Phabricator · Built by Phacility