Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

tls: does not emit 'end' from EncryptedStream #1936

Closed
wants to merge 1 commit into from
Closed

tls: does not emit 'end' from EncryptedStream #1936

wants to merge 1 commit into from

Conversation

koichik
Copy link

@koichik koichik commented Oct 25, 2011

de09168 and 4cdf9d4 breaks test/pummel/test-https-large-response.js.
It is never finished.

This patch is a workaround for a workaround, but works well both Linux and Windows.

de09168 and 4cdf9d4 breaks `test/pummel/test-https-large-response.js`.
It is never finished.
@bnoordhuis
Copy link
Member

test/pummel/test-https-large-response.js fails for me on master with ECONNREFUSED, with and without your patch. Will investigate.

Error: connect ECONNREFUSED
    at errnoException (net.js:589:11)
    at Object.afterConnect [as oncomplete] (net.js:580:18)

@bnoordhuis
Copy link
Member

Seems to be a IPv4-vs-IPv6 issue - the test works fine when you explicitly bind to 127.0.0.1 (see diff below).

Your patch fixes the hang with node but the funny thing is that node_g works fine without it. I don't quite see why that should be.

diff --git a/test/pummel/test-https-large-response.js b/test/pummel/test-https-large-response.js
index 13d8f61..6a9013c 100644
--- a/test/pummel/test-https-large-response.js
+++ b/test/pummel/test-https-large-response.js
@@ -52,8 +52,8 @@ var server = https.createServer(options, function(req, res) {
 var count = 0;
 var gotResEnd = false;

-server.listen(common.PORT, function() {
-  https.get({ port: common.PORT }, function(res) {
+server.listen(common.PORT, '127.0.0.1', function() {
+  https.get({ host: '127.0.0.1', port: common.PORT }, function(res) {
     console.log('response!');

     res.on('data', function(d) {

@koichik
Copy link
Author

koichik commented Oct 26, 2011

In my environment, both node and node_g are hung up without this patch :-<
This problem depends on the timing.
When the server calls CleartextStream.end(), then Socket.end() is called via Stream.pipe() (de09168).
However if Socket is paused (4cdf9d4), it is not closed immediately. It waits to be resumed.
But Stream.pipe() removes listeners from Socket, Socket.resume() (also Socket.destroy()) is not called. This is the reason why the test is not finished.

Probably, with node_g in your environment, the socket is not paused when Socket.end() is called.
I think that test/simple/test-tls-pause-close.js does not depend on the timing. Can you try it with/without this patch? (You may have to bind '127.0.0.1' explicitly too?)

@bnoordhuis
Copy link
Member

Go ahead and merge it, @koichik, your patch addresses the issue. To wit:

  1. test/pummel/test-https-large-response.js with or without your patch always fails with ECONNREFUSED (DNS issue)
  2. test/pummel/test-https-large-response.js that explicitly binds to 127.0.0.1 + your patch passes. Without your patch, node hangs while node_g passes.
  3. test/simple/test-tls-pause-close.js with your patch passes, no need to bind. Without your patch, node hangs while node_g passes.

That IPv4/IPv6 DNS bug is annoying but not related to this issue.

EDIT: by the way, s/EncriptedStream/EncryptedStream/

@koichik koichik closed this in 0e8a55d Oct 26, 2011
@koichik
Copy link
Author

koichik commented Oct 26, 2011

Thanks, I has fixed commit log and this title. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants