This repository has been archived by the owner. It is now read-only.

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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

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.

tls: does not emit 'end' from EncriptedStream
de09168 and 4cdf9d4 breaks `test/pummel/test-https-large-response.js`.
It is never finished.
Member

bnoordhuis commented Oct 25, 2011

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)
Member

bnoordhuis commented Oct 26, 2011

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 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?)

Member

bnoordhuis commented Oct 26, 2011

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 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.