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

Get thaliReplicationPeerAction to honor httpAgentPool #730

Closed
yaronyg opened this Issue Jun 3, 2016 · 2 comments

Comments

Projects
None yet
3 participants
@yaronyg
Member

yaronyg commented Jun 3, 2016

In order to work around pouchdb/pouchdb#5231 we need to pass in agentClass rather than agent (which is what we would have set with the httpAgentPool argument in start). So for right now we are manually specifying the agent class and options and ignoring httpAgentPool all together. If 5231 is fixed then we can use httpAgentPool, otherwise we need to redesign the action class so it submits the agentClass and options.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Jun 17, 2016

Member

As a note the PouchDB folks indicated that they would be interested in seeing a fix to this so when we have time I can submit one.

Member

yaronyg commented Jun 17, 2016

As a note the PouchDB folks indicated that they would be interested in seeing a fix to this so when we have time I can submit one.

@yaronyg

This comment has been minimized.

Show comment
Hide comment
@yaronyg

yaronyg Oct 31, 2016

Member

So it turns out that our work around for this bug is what causes thaliproject/Thali_CordovaPlugin#1406

What happens is that when we call secureOptions (see here for details) to work around this bug that this value eventually makes its way all the way down to OpenSSL where it is not properly formatted and so blows things up.

The only sane way to work around this is to fix pouchdb/pouchdb#5231. The bug happens as follows:

  1. Options get passed to https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js
  2. That function then calls https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js#L65 which calls https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-utils/src/clone.js which doesn't detect 'new' functions and so just tries to clone all the members of the function thus turning it from a function to an object and thus we lose the ability to be a function.

This blows us up because when we pass in our option which includes:

{
  ajax: {
     agent: actionAgent
   }
}

actionAgent, which is a new'd function, gets turned into an object and everything blows up.

The Fix

There are a couple of work arounds for this problem.

  1. We could just disable clone on anything for which 'typeof object === 'function''. This is a quick and easy hack but it's not something that PouchDB will ever accept as a PR. But right now I honestly don't know if I care. We can fix it later.
  2. We could put in code in https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js#L65 so that just before we call clone(opts) we detect if the opts object has opts.ajax.agent. If it does then we can copy the value of opts.ajax.agent to a temporary variable, call opts on the whole object and then copy the value back. E.g. something like:
var tempAgent = null;
if (opts.ajax && opts.ajax.agent) {
   tempAgent = opts.ajax.agent;
}

var cloneOpts = clone(opts);

if (tempAgent) {
   cloneOpts.ajax.agent = tempAgent;
}

this.__opts = opts = clone(opts);

They might actually accept this as the PouchDB folks were clear that they were willing to have special arguments for request since it's 'special'.

Testing the fix

Probably the easiest way to see if you have fixed it is to just run the repo I include at the top of pouchdb/pouchdb#5231. It is very short and completely self-contained. That test will fail on desktop if things aren't set up right.

Deploying the fix

The hardest part is that we need to re-activate installCustomPouchDB.js to build our own version of PouchDB. Now is NOT the time to try out the latest and greatest version of PouchDB. So I would suggest that we just fork version 5.4.5 of pouchdb (the last version we have tested with) and make the change in our fork and then re-activate installCustomPouchDB.installNodePouchDB and point it at your custom fork with the change and re-activate the code in installCustomPouchDB.installAll.

Note that you can find 5.4.5. of pouchDB at https://github.com/pouchdb/pouchdb/releases/tag/5.4.5. That has the commit ID we'll need.

Updating Thali

Once we have the fix then we need to update testUtils.createPskPouchDBRemote and thaliReplicationPeerAction.start and change the ajax options to use ajax.agent. Delete agentClass as well as agentOptions. Instead replace them with a call to new ForeverAgent.SSL. Put in the same options BUT REMOVE SECUREOPTIONS!!!!! You can see an example of this in testUtils.get.

Member

yaronyg commented Oct 31, 2016

So it turns out that our work around for this bug is what causes thaliproject/Thali_CordovaPlugin#1406

What happens is that when we call secureOptions (see here for details) to work around this bug that this value eventually makes its way all the way down to OpenSSL where it is not properly formatted and so blows things up.

The only sane way to work around this is to fix pouchdb/pouchdb#5231. The bug happens as follows:

  1. Options get passed to https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js
  2. That function then calls https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js#L65 which calls https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-utils/src/clone.js which doesn't detect 'new' functions and so just tries to clone all the members of the function thus turning it from a function to an object and thus we lose the ability to be a function.

This blows us up because when we pass in our option which includes:

{
  ajax: {
     agent: actionAgent
   }
}

actionAgent, which is a new'd function, gets turned into an object and everything blows up.

The Fix

There are a couple of work arounds for this problem.

  1. We could just disable clone on anything for which 'typeof object === 'function''. This is a quick and easy hack but it's not something that PouchDB will ever accept as a PR. But right now I honestly don't know if I care. We can fix it later.
  2. We could put in code in https://github.com/pouchdb/pouchdb/blob/master/packages/node_modules/pouchdb-core/src/constructor.js#L65 so that just before we call clone(opts) we detect if the opts object has opts.ajax.agent. If it does then we can copy the value of opts.ajax.agent to a temporary variable, call opts on the whole object and then copy the value back. E.g. something like:
var tempAgent = null;
if (opts.ajax && opts.ajax.agent) {
   tempAgent = opts.ajax.agent;
}

var cloneOpts = clone(opts);

if (tempAgent) {
   cloneOpts.ajax.agent = tempAgent;
}

this.__opts = opts = clone(opts);

They might actually accept this as the PouchDB folks were clear that they were willing to have special arguments for request since it's 'special'.

Testing the fix

Probably the easiest way to see if you have fixed it is to just run the repo I include at the top of pouchdb/pouchdb#5231. It is very short and completely self-contained. That test will fail on desktop if things aren't set up right.

Deploying the fix

The hardest part is that we need to re-activate installCustomPouchDB.js to build our own version of PouchDB. Now is NOT the time to try out the latest and greatest version of PouchDB. So I would suggest that we just fork version 5.4.5 of pouchdb (the last version we have tested with) and make the change in our fork and then re-activate installCustomPouchDB.installNodePouchDB and point it at your custom fork with the change and re-activate the code in installCustomPouchDB.installAll.

Note that you can find 5.4.5. of pouchDB at https://github.com/pouchdb/pouchdb/releases/tag/5.4.5. That has the commit ID we'll need.

Updating Thali

Once we have the fix then we need to update testUtils.createPskPouchDBRemote and thaliReplicationPeerAction.start and change the ajax options to use ajax.agent. Delete agentClass as well as agentOptions. Instead replace them with a call to new ForeverAgent.SSL. Put in the same options BUT REMOVE SECUREOPTIONS!!!!! You can see an example of this in testUtils.get.

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