Skip to content
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

switch dependency to http-proxy-node16 #539

Merged
merged 5 commits into from
Jun 10, 2024
Merged

switch dependency to http-proxy-node16 #539

merged 5 commits into from
Jun 10, 2024

Conversation

minrk
Copy link
Member

@minrk minrk commented May 23, 2024

seems to improve socket leak in #434

@minrk minrk mentioned this pull request May 23, 2024
@minrk minrk requested a review from consideRatio May 23, 2024 10:35
Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

This seems fine to me. The code diff between the HEAD of http-proxy and http-proxy-node16
is just https://github.com/Jimbly/http-proxy-node16/compare/9b96cd7..HEAD

diff --git a/lib/http-proxy/passes/web-incoming.js b/lib/http-proxy/passes/web-incoming.js
index 7ae7355..64da741 100644
--- a/lib/http-proxy/passes/web-incoming.js
+++ b/lib/http-proxy/passes/web-incoming.js
@@ -18,6 +18,11 @@ var nativeAgents = { http: httpNative, https: httpsNative };
  * flexible.
  */
 
+// 'aborted' event stopped working reliably on v15.5.0 and was later removed entirely
+var supportsAbortedEvent = (function () {
+  var ver = process.versions.node.split('.').map(Number);
+  return ver[0] <= 14 || ver[0] === 15 && ver[1] <= 4;
+}());
 
 module.exports = {
 
@@ -143,9 +148,18 @@ module.exports = {
     }
 
     // Ensure we abort proxy if request is aborted
-    req.on('aborted', function () {
-      proxyReq.abort();
-    });
+    if (supportsAbortedEvent) {
+      req.on('aborted', function () {
+        proxyReq.abort();
+      });
+    } else {
+      res.on('close', function () {
+        var aborted = !res.writableFinished;
+        if (aborted) {
+          proxyReq.abort();
+        }
+      });
+    }
 
     // handle errors in proxy and incoming request, just like for forward proxy
     var proxyError = createErrorHandler(proxyReq, options.target);
diff --git a/lib/http-proxy/passes/ws-incoming.js b/lib/http-proxy/passes/ws-incoming.js
index 270f23f..c38036b 100644
--- a/lib/http-proxy/passes/ws-incoming.js
+++ b/lib/http-proxy/passes/ws-incoming.js
@@ -129,7 +129,7 @@ module.exports = {
       // if it errors (eg, vanishes from the net and starts returning
       // EHOSTUNREACH). We need to do that explicitly.
       socket.on('error', function () {
-        proxySocket.end();
+        proxySocket.destroy();
       });
 
       common.setupSocket(proxySocket);

Relevant node issues are:

@minrk minrk merged commit 4f92c6a into jupyterhub:main Jun 10, 2024
11 checks passed
@minrk minrk deleted the fork branch June 10, 2024 11:39
@minrk minrk added the bug label Jun 10, 2024
@minrk minrk mentioned this pull request Jun 10, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants