Use http server over local domain socket instead of json-socket #3

etamponi · 2017-12-18T22:21:43Z

The code isn't great because it's my first attempt at TypeScript, but:

It now uses an http server which connects over the same unix/windows socket used by json-socket
I had to use express because fastify had issues during shutdown

Moreover, I found that the same problem that afflicted json-socket was causing issues with the http servers as well (included fastify): at the end I found that we were overloading the socket. In the retryRequest function you'll see that I put a simple backoff timeout if there are too many requests in flight. Probably we can port this backoff code back to json-socket so that it works too, if you like it more than the http approach.

etamponi · 2017-12-18T22:26:33Z

A couple more things worth mentioning:

I didn't run the test suite because I didn't know how to do that;
in retryRequest, the catch block is never taken, because the backoff that I put in front of the request makes sure all the requests run fine. You can try increasing the threshold number of inflight requests to see the errors happening.

zkochan · 2017-12-18T23:53:58Z

package.json

-    "json-socket": "^0.3.0",
+    "express": "^4.16.2",
+    "body-parser": "^1.18.2",
+    "fastify": "^0.35.7",


it doesn't seem like you use fastify in code

zkochan · 2017-12-18T23:55:27Z

src/connectStoreController.ts

+      setTimeout(resolve, 100)
+    }).then(() => {
+      return retryRequest(options)
+    })


you can use p-limit

zkochan · 2017-12-18T23:57:14Z

src/connectStoreController.ts

-import JsonSocket = require('json-socket')
-import net = require('net')
+
+import request = require('request-promise-native')


use node-fetch instead

zkochan · 2017-12-19T09:00:28Z

src/createServer.ts

-          err,
-        }, (merr) => merr && console.error(merr))
-      })
+function garbageCollectResponses (packageResponses: object, msgId: string) {


you can have 2 separate dictionaries and you won't need this function

zkochan · 2017-12-19T09:04:03Z

src/createServer.ts

+    limit: '10mb',
+  }))
+
+  app.post('/requestPackage', async (request: Request, response: Response) => {


instead of the separate actions, it might be faster to have a single route with a switch inside.

etamponi · 2017-12-19T10:44:08Z

I made the following changes:

Removed express, now using the raw http server and a switch clause -- it supports errors too now
Added p-limit
Split packageResponses into two separate dictionaries

The only thing I didn't do is replacing request-promise-native with node-fetch, as I wasn't able to figure out how node-fetch handles error conditions. The docs say it would throw a FetchError, but in my tests: https://glitch.com/edit/#!/fastify-test?path=server.js:56:15 it does not happen.

Is node-fetch better, or can we leave request-promise-native as it seems it is working quite well? (We also use it on the Glitch source code without any issues).

zkochan · 2017-12-19T10:59:54Z

The reason I asked to remove the request usage is because we had an issue with it. See pnpm/pnpm#953 . The reason I asked node-fetch is because we already use it.

…

On Tue, Dec 19, 2017, 12:44 Emanuele Tamponi ***@***.***> wrote: I made the following changes: - Removed express, now using the raw http server and a switch clause -- it supports errors too now - Added p-limit - Split packageResponses into two separate dictionaries The only thing I didn't do is replacing request-promise-native with node-fetch, as I wasn't able to figure out how node-fetch handles error conditions. The docs say it would throw a FetchError, but in my tests: https://glitch.com/edit/#!/fastify-test?path=server.js:56:15 it does not happen. Is node-fetch better, or can we leave request-promise-native as it seems it is working quite well? (We also use it on the Glitch source code without any issues). — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1pm9-az1UO1VXVIbxOayvf-hgsqQy1ks5tB5N4gaJpZM4RGKDW> .

etamponi · 2017-12-19T11:03:26Z

Thanks, I'll make sure I replace it with node-fetch then! Can you help me figure out how error conditions are handled by node-fetch? If the remote resource returns a 500, shouldn't the fetch promise be rejected? I see it being resolved instead... Il 19 dic 2017 11:59, "Zoltan Kochan" <notifications@github.com> ha scritto:

…

The reason I asked to remove the request usage is because we had an issue with it. See pnpm/pnpm#953 . The reason I asked node-fetch is because we already use it. On Tue, Dec 19, 2017, 12:44 Emanuele Tamponi ***@***.***> wrote: > I made the following changes: > > - Removed express, now using the raw http server and a switch clause > -- it supports errors too now > - Added p-limit > - Split packageResponses into two separate dictionaries > > The only thing I didn't do is replacing request-promise-native with > node-fetch, as I wasn't able to figure out how node-fetch handles error > conditions. The docs say it would throw a FetchError, but in my tests: > https://glitch.com/edit/#!/fastify-test?path=server.js:56:15 it does not > happen. > > Is node-fetch better, or can we leave request-promise-native as it seems > it is working quite well? (We also use it on the Glitch source code without > any issues). > > — > You are receiving this because your review was requested. > > > Reply to this email directly, view it on GitHub > <#3 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe- auth/AB1pm9-az1UO1VXVIbxOayvf-hgsqQy1ks5tB5N4gaJpZM4RGKDW> > . > -- Zoltan — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAjUNL16ByDJMH5j_0AiwLoaHiKypAdFks5tB5crgaJpZM4RGKDW> .

zkochan · 2017-12-19T11:36:40Z

Probably you have to check status codes. Seems like it throws an error only when a request couldn't be done. A 500 response is still a completed request. On Tue, Dec 19, 2017, 13:03 Emanuele Tamponi <notifications@github.com> wrote:

…

Thanks, I'll make sure I replace it with node-fetch then! Can you help me figure out how error conditions are handled by node-fetch? If the remote resource returns a 500, shouldn't the fetch promise be rejected? I see it being resolved instead... Il 19 dic 2017 11:59, "Zoltan Kochan" ***@***.***> ha scritto: > The reason I asked to remove the request usage is because we had an issue > with it. See pnpm/pnpm#953 . The reason I asked > node-fetch is because we already use it. > > On Tue, Dec 19, 2017, 12:44 Emanuele Tamponi ***@***.***> > wrote: > > > I made the following changes: > > > > - Removed express, now using the raw http server and a switch clause > > -- it supports errors too now > > - Added p-limit > > - Split packageResponses into two separate dictionaries > > > > The only thing I didn't do is replacing request-promise-native with > > node-fetch, as I wasn't able to figure out how node-fetch handles error > > conditions. The docs say it would throw a FetchError, but in my tests: > > https://glitch.com/edit/#!/fastify-test?path=server.js:56:15 it does not > > happen. > > > > Is node-fetch better, or can we leave request-promise-native as it seems > > it is working quite well? (We also use it on the Glitch source code > without > > any issues). > > > > — > > You are receiving this because your review was requested. > > > > > > Reply to this email directly, view it on GitHub > > <#3 (comment)>, or mute > > the thread > > <https://github.com/notifications/unsubscribe- > auth/AB1pm9-az1UO1VXVIbxOayvf-hgsqQy1ks5tB5N4gaJpZM4RGKDW> > > . > > > -- > > Zoltan > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#3 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AAjUNL16ByDJMH5j_0AiwLoaHiKypAdFks5tB5crgaJpZM4RGKDW > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#3 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1pm7pMfl6P-Iy3f6vVmGvXg7JClrReks5tB5f-gaJpZM4RGKDW> .

…sockets

etamponi · 2017-12-19T15:01:10Z

Unfortunately I couldn't use node-fetch because it does not support unix sockets, apparently just for the sake of not supporting them. The node-fetch maintainer suggests using got instead, so I replaced request-promise-native with got.

You're right: request was causing some issues, including the random ECONNRESET errors. With got, in theory, we can remove the limit on the number of inflight requests. I think it is better to leave it though.

etamponi · 2017-12-19T15:12:46Z

With the last commit I made the tests pass on my local machine, I hope travis is happy too :)

UPDATE: looks like Node 4 doesn't like my changes, I don't know why...

UPDATE 2: Fixed tests on Node 4 too :)

zkochan · 2017-12-19T15:17:21Z

Well done! I'll review this tonight

zkochan · 2017-12-19T19:24:24Z

package.json

@@ -38,7 +38,11 @@
    "@pnpm/logger": "^1.0.0",
    "@pnpm/npm-resolver": "^0.3.1",
    "@pnpm/tarball-fetcher": "^0.3.1",
+    "@types/got": "^7.1.6",


type dependencies of prod dependencies should be also prod dependencies

zkochan · 2017-12-19T19:26:34Z

src/connectStoreController.ts

+  } else {
+    remotePrefix = `http://${initOpts.hostname}:${initOpts.port}`
+  }
+  const limitedRetryFetch = retryFetch.bind(null, pLimit(100))


I'd make the concurrency configurable

zkochan · 2017-12-19T19:27:32Z

src/connectStoreController.ts

+  if (initOpts.path) {
+    remotePrefix = `http://unix:${initOpts.path}:`
+  } else {
+    remotePrefix = `http://${initOpts.hostname}:${initOpts.port}`


I think it would be fine to pass in the formed path, these manipulations can be done by pnpm

zkochan · 2017-12-19T19:30:18Z

src/connectStoreController.ts

+      method: 'POST',
+    })
+    return JSON.parse(response.body)
+  }).catch((e) => {


you can use async/await syntax here with a regular try/catch

also, I think the try/catch should be inside the function and cover just the got() call, as errors by JSON.parse will always be passed through

zkochan · 2017-12-19T19:32:53Z

src/connectStoreController.ts

+    if (!e.message.startsWith('Error: connect ECONNRESET') && !e.message.startsWith('Error: connect ECONNREFUSED')) {
+      throw e
+    }
+    return retryFetch(limit, url, body)


got() supports retries out of the box, so I guess this logic is no longer needed.

and is there a way to tell got to retry as many times as it's needed?

yes, by default it reties 2 times
https://www.npmjs.com/package/got#retries

zkochan · 2017-12-19T19:35:34Z

src/createServer.ts

-          await store.saveState()
-          return
+
+        switch (req.url) {


the switch statement should be probably executed early, because if an unknown route is requested, there's no need to read the data

The switch cannot be executed early because we don't have the body until the request has been received, and I don't see any reason to add complexity to wait for the body since we always need the body or it is empty so it doesn't add overhead.

isn't the URL known from the very start? The URL can be validated before collecting the body

and once we validate the URL? We've to wait for the body... so I am not really sure why we should validate the URL first

zkochan · 2017-12-19T19:36:42Z

src/createServer.ts

+          case '/saveState':
+            await store.saveState()
+            res.end(JSON.stringify('OK'))
+            break
        }


if no route was matched, I think a 404 response should be returned

zkochan · 2017-12-19T19:49:05Z

It might be a good idea to prefix the routes with a version, like /v0/requestPackage. In that case we'll be able to make breaking changes more smoothly. Though it is not necessary in scope of this PR. The feature is experimental for now, anyway.

zkochan · 2017-12-19T19:54:14Z

Where are the changes in shrinkwrap.yaml?

etamponi · 2017-12-19T19:54:58Z

EDIT: I was using npm install, I'll use pnpm install to generate it

zkochan · 2017-12-19T19:55:57Z

It is updated automatically when you install/uninstall with pnpm.

etamponi · 2017-12-19T20:15:14Z

Just pushed the fixes following your suggestions:

now using retries in got, setting it to 1000 so that we're sure we retry enough times; got rid of the catch block
pLimit argument is now configurable through initOpts.concurrency, defaulting to 100
updated shrinkwrap.yaml
initOpts now has remotePrefix, the manipulations need to be done in pnpm now
the request body is now fetched in a promise, bodyPromise, and we do the URL validation before the body has been fully received. We then await for the bodyPromise when needed.
added a 404 catch-all route

zkochan · 2017-12-19T20:18:42Z

src/connectStoreController.ts

      method: 'POST',
+      retries: 1000,  // picking a very large number to be sure it keeps retrying as long as needed


I am not sure this is a good idea because, with each retry, the delay is increased. So eventually it will just hang up. It can be considered a silent error

I'll pass a function with a fixed delay instead

zkochan · 2017-12-19T20:43:11Z

Sorry, last 2 requests.

@types/node should be prod dependency as well
unite the changes to one commit and name it, using our conventions: feat: use http over local domain socket for communication

etamponi · 2017-12-19T20:57:41Z

I closed this PR and created a new one with the changes and commit you asked

zkochan · 2017-12-20T09:44:39Z

These changes don't work with Windows. The previous implementation worked on Windows

etamponi · 2017-12-20T09:48:25Z

What exactly doesn't work? Probably the remotePrefix string needs to be written correctly for Windows

zkochan · 2017-12-20T09:51:43Z

Yes, that might be the case, as the URL contains http://unix: https://github.com/pnpm/pnpm/blob/next/src/cmd/server.ts#L34

etamponi · 2017-12-20T09:58:33Z

I don't have access to a Windows machine unfortunately, I can ask a coworker later on though. In the meantime, it should be possible to use localhost connections on Windows instead of a socket.

zkochan · 2017-12-20T10:04:37Z

We have appveyor tests on windows. They fail now https://ci.appveyor.com/project/zkochan/pnpm-17nv8/build/2015

I can also look into it, but only in the evening

zkochan · 2017-12-20T21:22:32Z

I fixed it by using localhost connection but it may have to be changed back to IPC in the future, for consistency and speed, on all systems

Emanuele Tamponi added 2 commits December 18, 2017 23:15

use http server with express over local socket

528b615

Merge remote-tracking branch 'upstream/master'

77cdb73

etamponi mentioned this pull request Dec 18, 2017

pnpm as a service pnpm/pnpm#832

Closed

zkochan self-requested a review December 18, 2017 23:27

zkochan reviewed Dec 18, 2017

View reviewed changes

src/connectStoreController.ts Outdated

setTimeout(resolve, 100)

}).then(() => {

return retryRequest(options)

})

Copy link

Member

zkochan Dec 18, 2017

Choose a reason for hiding this comment

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

you can use p-limit

zkochan reviewed Dec 18, 2017

View reviewed changes

zkochan reviewed Dec 19, 2017

View reviewed changes

change code following review suggestions

abaeb7a

use got instead of request, because node-fetch does not support unix …

9b51d54

…sockets

support hostname/port too, make tests pass

b08f7a5

fixed tests for Node 4

58c63bb

zkochan reviewed Dec 19, 2017

View reviewed changes

zkochan suggested changes Dec 19, 2017

View reviewed changes

review suggestions

70ab908

zkochan reviewed Dec 19, 2017

View reviewed changes

use a fixed retry delay instead

4fb945c

zkochan approved these changes Dec 19, 2017

View reviewed changes

tslint fix

9ccf8a9

etamponi closed this Dec 19, 2017

		method: 'POST',
		retries: 1000, // picking a very large number to be sure it keeps retrying as long as needed

Use http server over local domain socket instead of json-socket #3

Use http server over local domain socket instead of json-socket #3

Conversation

etamponi commented Dec 18, 2017

etamponi commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etamponi commented Dec 19, 2017

zkochan commented Dec 19, 2017 via email • edited

etamponi commented Dec 19, 2017 via email

zkochan commented Dec 19, 2017 via email • edited

etamponi commented Dec 19, 2017 • edited

etamponi commented Dec 19, 2017 • edited

zkochan commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Dec 19, 2017

zkochan commented Dec 19, 2017

etamponi commented Dec 19, 2017 • edited

zkochan commented Dec 19, 2017

etamponi commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zkochan commented Dec 19, 2017

etamponi commented Dec 19, 2017

zkochan commented Dec 20, 2017

etamponi commented Dec 20, 2017

zkochan commented Dec 20, 2017

etamponi commented Dec 20, 2017

zkochan commented Dec 20, 2017

zkochan commented Dec 20, 2017

zkochan commented Dec 19, 2017 via email •

edited

zkochan commented Dec 19, 2017 via email •

edited

etamponi commented Dec 19, 2017 •

edited

etamponi commented Dec 19, 2017 •

edited

etamponi commented Dec 19, 2017 •

edited