Skip to content
This repository was archived by the owner on Feb 12, 2024. It is now read-only.

feat(issue-1852): now supports multiple api and gateways #1903

Merged
alanshaw merged 5 commits into from
Mar 11, 2019
Merged

feat(issue-1852): now supports multiple api and gateways #1903

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5c7628
feat(issue-1852): now supports multiple api and gateways
grantlouisherman Feb 28, 2019
1b17688
feat(pr): addressing PR comments
grantlouisherman Mar 7, 2019
e57d0e9
refactor: cleanup and add test
Mar 11, 2019
1c38202
fix: guard against undefined _apiServers
Mar 11, 2019
d5215c2
chore: increase timeout for windows
Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 44 additions & 17 deletions src/http/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ function hapiInfoToMultiaddr (info) {
return toMultiaddr(uri)
}

async function serverCreator (serverAddrsArr, createServerFunc, hapiInfoToMultiaddr, ipfs) {

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No need to pass hapiInfoToMultiaddr, it is in scope

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please can we rename serverAddrsArr to just serverAddrs and then add a check as the first line of this function to convert it to an array if it is not already:

serverAddrs = Array.isArray(serverAddrs) ? serverAddrs : [serverAddrs]

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please can we rename createServerFunc to createServer? By convention we don't add type information to our variable names in this project.

if (!serverAddrsArr.length) {
debug(Error('There are no addresses'))

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please just return [] here instead - it shouldn't be an error to not have any addresses to listen on.

}
// just in case the address is just string
let serversAddrs = [].concat(serverAddrsArr)

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be removed if we add the check as suggested above.

const processServer = async (serverInstance, createServerFunc, hapiInfoToMultiaddr, ipfs) => {

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rename serverInstance => addr/address, remove createServerFunc, hapiInfoToMultiaddr, ipfs - these are all already in scope.

let addr = serverInstance.split('/')

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be const. Please also rename to addrParts or something similar.

let _Server = await createServerFunc(addr[2], addr[4], ipfs)

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can be const. Please also rename to server.

await _Server.start()
_Server.info.ma = hapiInfoToMultiaddr(_Server.info)
return _Server
}
return Promise.all(
serversAddrs.map(server => processServer(server, createServerFunc, hapiInfoToMultiaddr, ipfs))
).catch(err => debug(err))

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please do not catch this error - it is fatal.

This can become:

return Promise.all(serverAddrs.map(processServer))

}

class HttpApi {
constructor (options) {
this._options = options || {}
Expand Down Expand Up @@ -89,24 +107,28 @@ class HttpApi {

const config = await ipfs.config.get()

const apiAddr = config.Addresses.API.split('/')
const apiServer = await this._createApiServer(apiAddr[2], apiAddr[4], ipfs)
await apiServer.start()
apiServer.info.ma = hapiInfoToMultiaddr(apiServer.info)
this._apiServer = apiServer
const apiAddrs = config.Addresses.API

this._apiServer = await Promise.resolve(

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename this to this._apiServers to indicate that there may be many.

serverCreator.apply(this, [apiAddrs, this._createApiServer, hapiInfoToMultiaddr, ipfs])

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why await Promise.resolve and serverCreator.apply? You can just call this function and await it:

this._apiServers = await serverCreator(apiAddrs, this._createApiServer, ipfs)

)
// for the CLI to know the where abouts of the API
await promisify(ipfs._repo.apiAddr.set)(apiServer.info.ma)
await promisify(ipfs._repo.apiAddr.set)(this._apiServer[0].info.ma)

const gatewayAddr = config.Addresses.Gateway.split('/')
const gatewayServer = await this._createGatewayServer(gatewayAddr[2], gatewayAddr[4], ipfs)
await gatewayServer.start()
gatewayServer.info.ma = hapiInfoToMultiaddr(gatewayServer.info)
this._gatewayServer = gatewayServer
const gatewayAddr = config.Addresses.Gateway

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename gatewayAddrs


ipfs._print('API listening on %s', apiServer.info.ma)
ipfs._print('Gateway (read only) listening on %s', gatewayServer.info.ma)
ipfs._print('Web UI available at %s', toUri(apiServer.info.ma) + '/webui')
this._gatewayServer = await Promise.resolve(
serverCreator.apply(this, [gatewayAddr, this._createGatewayServer, hapiInfoToMultiaddr, ipfs])
)

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same here, please rename to this._gatewayServers and just await the result:

this._gatewayServer = await serverCreator(gatewayAddrs, this._createGatewayServer, ipfs)

this._apiServer.forEach(apiServer => {
ipfs._print('API listening on %s', apiServer.info.ma)
})
this._gatewayServer.forEach(gatewayServer => {
ipfs._print('Gateway (read only) listening on %s', gatewayServer.info.ma)
})
this._apiServer.forEach(apiServer => {
ipfs._print('Web UI available at %s', toUri(apiServer.info.ma) + '/webui')
})
this._log('started')
return this
}
Expand Down Expand Up @@ -177,14 +199,19 @@ class HttpApi {

get apiAddr () {
if (!this._apiServer) throw new Error('API address unavailable - server is not started')
return multiaddr('/ip4/127.0.0.1/tcp/' + this._apiServer.info.port)
return multiaddr('/ip4/127.0.0.1/tcp/' + this._apiServer[0].info.port)
}

async stop () {
function stopServer (serverArr) {

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename stopServer -> stopServers and serverArr to servers.

for (let i = 0; i < serverArr.length; i++) {
serverArr[i].stop()
}

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You need to return a promise from this function:

function stopServers (servers) {
  return Promise.all(servers.map(server => server.stop()))
}

}
this._log('stopping')
await Promise.all([
this._apiServer && this._apiServer.stop(),
this._gatewayServer && this._gatewayServer.stop(),
this._apiServer && stopServer(this._apiServer),

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
this._apiServer && stopServer(this._apiServer),
stopServers(this._apiServers),

this._gatewayServer && stopServer(this._gatewayServer),

@alanshaw alanshaw Mar 6, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
this._gatewayServer && stopServer(this._gatewayServer),
stopServers(this._gatewayServer),

this._ipfs && this._ipfs.stop()
])
this._log('stopped')
Expand Down
2 changes: 1 addition & 1 deletion test/gateway/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe('HTTP Gateway', function () {

await http.api.start()

gateway = http.api._gatewayServer
gateway = http.api._gatewayServer[0]

// QmbQD7EMEL1zeebwBsWEfA3ndgSS6F7S6iTuwuqasPgVRi
await http.api._ipfs.add([
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/bitswap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

before(async function () {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('/block/put', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
return api.inject({
method: 'GET',
url: '/api/v0/bootstrap/add/default'
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = (http) => {

before(() => {
updatedConfig = () => JSON.parse(fs.readFileSync(configPath, 'utf8'))
api = http.api._apiServer
api = http.api._apiServer[0]
})

after(() => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/dht.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('/findpeer', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/dns.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('resolve ipfs.io dns', async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/files.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('/add', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/id.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('get the id', async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('should publish a record', async function () {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/object.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ module.exports = (http) => {
let api

before('api', () => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('/new', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/pin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('rm', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/ping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('returns 400 if both n and count are provided', async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/pubsub.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ module.exports = (http) => {
const topicNotSubscribed = 'somethingRandom'

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

describe('/sub', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/resolve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('should resolve a path and return a base2 encoded CID', async () => {
Expand Down
2 changes: 1 addition & 1 deletion test/http-api/inject/version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ module.exports = (http) => {
let api

before(() => {
api = http.api._apiServer
api = http.api._apiServer[0]
})

it('get the version', async () => {
Expand Down