wip: simplify chain #156

szmarczak · 2021-09-13T10:46:54Z

this PR

simplify chain
- remove handler base

wip: simplify forward

wip: simplify custom response

simplify custom response
use shared eslint config

wip: simplify direct

simplify direct

TODO (those will be separate PRs)

B4nan · 2021-09-13T10:58:13Z

src/server.js

                }

                this.log(handlerOpts.id, 'Using HandlerTunnelDirect');
-                return direct(request, socket, head);
+                return direct(request, socket, head, handlerOpts, this);


looks like there are just the 3 parameters on direct?

now it has 4 params, and you are feeding 5 (and server is 5th, not 4th), right?

Ah, this one :) Now it's 5, handlerOpts won't be used but I prefer to keep this one so it looks the same like chain.

szmarczak · 2021-09-13T11:10:36Z

https://github.com/apify/proxy-chain/runs/3586133160

  1) tcp_tunnel.createTunnel
       creates tunnel that is able to transfer data:
     Uncaught Error: listen EADDRINUSE: address already in use :::38102
      at Server.setupListenHandle [as _listen2] (node:net:1319:16)
      at listenInCluster (node:net:1367:12)
      at Server.listen (node:net:1454:7)
      at /home/runner/work/proxy-chain/proxy-chain/test/tcp_tunnel.js:19:12
      at new Promise (<anonymous>)
      at serverListen (test/tcp_tunnel.js:18:40)
      at /home/runner/work/proxy-chain/proxy-chain/test/tcp_tunnel.js:154:24
      at tryCatcher (node_modules/bluebird/js/main/util.js:26:23)
      at Promise._settlePromiseFromHandler (node_modules/bluebird/js/main/promise.js:510:31)
      at Promise._settlePromiseAt (node_modules/bluebird/js/main/promise.js:584:18)
      at Async._drainQueue (node_modules/bluebird/js/main/async.js:128:12)
      at Async._drainQueues (node_modules/bluebird/js/main/async.js:133:10)
      at Immediate.Async.drainQueues (node_modules/bluebird/js/main/async.js:15:14)
      at processImmediate (node:internal/timers:464:21)

@mnmkng I just got the error @rubydev saw before, looks like Node.js selected a protocol that was already in use?

B4nan

idea for future PRs - maybe we should turn the 5 parameters into object

mnmkng · 2021-09-13T20:36:55Z

https://github.com/apify/proxy-chain/runs/3586133160


  1) tcp_tunnel.createTunnel

       creates tunnel that is able to transfer data:

     Uncaught Error: listen EADDRINUSE: address already in use :::38102

      at Server.setupListenHandle [as _listen2] (node:net:1319:16)

      at listenInCluster (node:net:1367:12)

      at Server.listen (node:net:1454:7)

      at /home/runner/work/proxy-chain/proxy-chain/test/tcp_tunnel.js:19:12

      at new Promise (<anonymous>)

      at serverListen (test/tcp_tunnel.js:18:40)

      at /home/runner/work/proxy-chain/proxy-chain/test/tcp_tunnel.js:154:24

      at tryCatcher (node_modules/bluebird/js/main/util.js:26:23)

      at Promise._settlePromiseFromHandler (node_modules/bluebird/js/main/promise.js:510:31)

      at Promise._settlePromiseAt (node_modules/bluebird/js/main/promise.js:584:18)

      at Async._drainQueue (node_modules/bluebird/js/main/async.js:128:12)

      at Async._drainQueues (node_modules/bluebird/js/main/async.js:133:10)

      at Immediate.Async.drainQueues (node_modules/bluebird/js/main/async.js:15:14)

      at processImmediate (node:internal/timers:464:21)

@mnmkng I just got the error @rubydev saw before, looks like Node.js selected a protocol that was already in use?

@szmarczak That's weird. Could there be race condition somewhere?

jancurn · 2021-09-13T20:44:48Z

That's weird. Could there be race condition somewhere?

In the past we used to pick randomized ports, which was a race condition. Now it should be done by operating system, hopefully without races... so this is strange, unless this is an error from an older version?

jancurn · 2021-09-13T20:54:09Z

Looking at all these PRs, I'm increasingly more worried that this big rewrite can potentially bring a lot of problems to this module that we rely on so heavily. There were a lot of iterations and a lot of real-world testing that went into this module to make sure it was stable and performing well (in particular the socket lifecycle), and now I feel we might be back to square one and might have to go through all the (hard-to-find) errors again, potentially with a big business impact (think of all potential problems a buggy Apify Proxy can cause to our customers).

I better stop watching these PRs and I truly hope you guys know what you're doing. Please make sure to write really good comments for the functions, so that other people can understand what's going on.

jancurn · 2021-09-13T20:55:31Z

src/chain.js

+    };
+
+    if (proxy.username || proxy.password) {
+        const auth = `${proxy.username}:${proxy.password}`;


What is either username or password is null? Then the auth string will contain "null".

In WHATWG URL it's never null, it's an empty string.

With TypeScript we won't have to worry can this be null? :)

szmarczak · 2021-09-13T21:35:05Z

Could there be race condition somewhere?

The tests still use

proxy-chain/test/tools.js

Lines 18 to 34 in 0043ac9

    
           const findFreePort = () => { 
        
               // Let 'min' be a random value in the first half of the PORT_FROM-PORT_TO range, 
        
               // to reduce a chance of collision if other ProxyChain is started at the same time. 
        
               const half = Math.floor((PORT_SELECTION_CONFIG.TO - PORT_SELECTION_CONFIG.FROM) / 2); 
        
               const opts = { 
        
                   min: PORT_SELECTION_CONFIG.FROM + Math.floor(Math.random() * half), 
        
                   max: PORT_SELECTION_CONFIG.TO, 
        
                   retrieve: 1, 
        
               }; 
        
               return portastic.find(opts) 
        
                   .then((ports) => { 
        
                       if (ports.length < 1) throw new Error(`There are no more free ports in range from ${PORT_SELECTION_CONFIG.FROM} to ${PORT_SELECTION_CONFIG.TO}`); // eslint-disable-line max-len 
        
                       return ports[0]; 
        
                   }); 
        
           };

so I guess it generated an in-use port. I'm starting to think @rubydev's error is unrelated to this one.

now I feel we might be back to square one and might have to go through all the (hard-to-find) errors again

Currently all the tests pass (except the two ones with stats, which will be fixed later). The point of the rewrite is so we don't have to mutate the socket if there's no need to. Only operate on objects we really have to. So if we receive a normal HTTP/1.1 request, let's operate only on request and response objects, no need to tinker with socket. This way we'll prevent event emitter memory leaks and the code will be more readable.

There were a lot of iterations and a lot of real-world testing that went into this module to make sure it was stable and performing well

We can gradually roll out the rewrite. Initially e.g. 1% of all requests can go through the rewrite, the next day it can be 5%, the next day it can be 10% etc.

in particular the socket lifecycle

What about it? As I see it, it remains the same. The socket gets closed only when a user does so or there's an error on any side. Am I missing something? If you have any questions, I'm happy to answer them.

I better stop watching these PRs and I truly hope you guys know what you're doing.

I'd prefer if you reviewed the code, you are the creator, but I'm not insisting. This can wait, but needs to be done at some point. I'm trying to keep the code as simple as possible.

szmarczak · 2021-09-13T21:40:57Z

I've added more descriptive errors on my todo list. So if an error gets thrown, we'll know all the details, upstream proxy, status code, request options etc.

jancurn · 2021-09-13T21:51:16Z

Thanks @szmarczak it was just a bit of an old man's afraidness of change that was speaking :) Of course, I'll have a look at the PRs, I'll do a deeper review of the big PR once it's ready. I'm confident your changes will make the module better and cleaner. Just pls comment the functions well, to make it easier to review.

Regarding the socket life-cycle, there were some rare strange races that "should never happen" but indeed happened, which led to code blocks like:

// See https://github.com/apify/proxy-chain/issues/63
if (this.isClosed) return;

These come from battle-testing, so it might happen they will reoccur. But let's see...

The gradual roll-out is a good idea, once this is ready pls chat with the Platform team how to do it.

szmarczak · 2021-09-13T22:11:03Z

it was just a bit of an old man's afraidness of change that was speaking :)

I understand the worry :)

I'll have a look at the PRs, I'll do a deeper review of the big PR once it's ready.

Thanks a lot ❤️ Two heads always better than one :D

Just pls comment the functions well, to make it easier to review.

👍🏼

which led to code blocks like

This is also what I'm trying to prevent. HandlerBase created a response on top of a detached socket (from CONNECT method) from the native http module:

proxy-chain/src/handler_base.js

Lines 43 to 49 in 0043ac9

    
           if (!this.srcResponse) { 
        
               this.srcResponse = new http.ServerResponse(srcRequest); 
        
               this.srcResponse.shouldKeepAlive = false; 
        
               this.srcResponse.chunkedEncoding = false; 
        
               this.srcResponse.useChunkedEncodingByDefault = false; 
        
               this.srcResponse.assignSocket(this.srcSocket); 
        
           }

therefore there were more critical points that needed to be handled.

szmarczak · 2021-09-13T22:20:34Z

// See https://github.com/apify/proxy-chain/issues/63
if (this.isClosed) return;

There was a race condition. Before the end socket gets connected the client one to the proxy may get destroyed (e.g. via disconnect). If I understand this right, the this.isClosed checks prevent this.

I think this PR is good to go. Ready to merge when you are :)

wip: simplify chain

fb1a1b4

szmarczak requested review from B4nan, jancurn and mnmkng September 13, 2021 10:47

B4nan reviewed Sep 13, 2021

View reviewed changes

fix: logging

b8644c3

fix: params

5e6ccf7

szmarczak requested a review from B4nan September 13, 2021 12:33

B4nan approved these changes Sep 13, 2021

View reviewed changes

jancurn reviewed Sep 13, 2021

View reviewed changes

szmarczak added 3 commits September 14, 2021 00:30

fix: race condition

ac27009

More sanity checks

b1248ed

log

3b27b71

mnmkng approved these changes Sep 14, 2021

View reviewed changes

szmarczak merged commit fb2c412 into next Sep 14, 2021

szmarczak deleted the simplify-chain branch September 14, 2021 12:18

This was referenced Sep 14, 2021

wip: simplify tools #157

Merged

wip: simplify tunnel #158

Merged

wip: simplify server #159

Merged

szmarczak mentioned this pull request Sep 27, 2021

wip: move to TypeScript #161

Merged

30 tasks

szmarczak mentioned this pull request Oct 6, 2021

release: 2.0.0 #162

Merged

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip: simplify chain #156

wip: simplify chain #156

szmarczak commented Sep 13, 2021 •

edited

Loading

B4nan Sep 13, 2021

szmarczak Sep 13, 2021

B4nan Sep 13, 2021

szmarczak Sep 13, 2021

szmarczak commented Sep 13, 2021 •

edited

Loading

B4nan left a comment

mnmkng commented Sep 13, 2021

jancurn commented Sep 13, 2021

jancurn commented Sep 13, 2021

jancurn Sep 13, 2021

szmarczak Sep 13, 2021

szmarczak Sep 13, 2021

szmarczak commented Sep 13, 2021 •

edited

Loading

szmarczak commented Sep 13, 2021

jancurn commented Sep 13, 2021

szmarczak commented Sep 13, 2021

szmarczak commented Sep 13, 2021 •

edited

Loading

wip: simplify chain #156

wip: simplify chain #156

Conversation

szmarczak commented Sep 13, 2021 • edited Loading

this PR

TODO (those will be separate PRs)

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

szmarczak commented Sep 13, 2021 • edited Loading

B4nan left a comment

Choose a reason for hiding this comment

mnmkng commented Sep 13, 2021

jancurn commented Sep 13, 2021

jancurn commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Sep 13, 2021 • edited Loading

szmarczak commented Sep 13, 2021

jancurn commented Sep 13, 2021

szmarczak commented Sep 13, 2021

szmarczak commented Sep 13, 2021 • edited Loading

szmarczak commented Sep 13, 2021 •

edited

Loading

szmarczak commented Sep 13, 2021 •

edited

Loading

szmarczak commented Sep 13, 2021 •

edited

Loading

szmarczak commented Sep 13, 2021 •

edited

Loading