Skip to content

tls: re-enable .writev() on TLSWrap - #1155

Closed
indutny wants to merge 1 commit into
nodejs:v1.xfrom
indutny:fix/tls-over-http-on-windows
Closed

tls: re-enable .writev() on TLSWrap#1155
indutny wants to merge 1 commit into
nodejs:v1.xfrom
indutny:fix/tls-over-http-on-windows

Conversation

@indutny

Copy link
Copy Markdown
Member

Fix the parallel/test-tls-over-http-tunnel.js on Windows by
re-enabling the accidentally disabled .writev() method on TLSWrap.

It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent shutdown/destroy, which breaks the test.

cc @piscisaureus

It appears that removing writev in io.js prior to StreamBase does not introduce this problem, but this patch fixes the problem without touching the test. I propose landing it, and meanwhile I'll investigate more.

Fix the `parallel/test-tls-over-http-tunnel.js` on Windows by
re-enabling the accidentally disabled `.writev()` method on TLSWrap.
It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent `shutdown`/`destroy`, which breaks the test.
@indutnyindutny mentioned this pull request Mar 14, 2015
@indutny

Copy link
Copy Markdown
MemberAuthor

UPDATE: I was able to reproduce this problem on io.js prior to StreamBase changes after removing .cork()/.uncork() from _http_outgoing.js. So this is definitely unrelated.

@indutny

Copy link
Copy Markdown
MemberAuthor

cc @iojs/crypto @bnoordhuis seems to be an obvious change

@piscisaureus

Copy link
Copy Markdown
Contributor

@indutny@indutny Interesting. What I found is that the number of callbacks that node expects (stream._wriableState.pendingcb) doesn't match what libuv thinks.

@indutny

Copy link
Copy Markdown
MemberAuthor

This is fine, because they are queued in TLSWrap

On Saturday, March 14, 2015, Bert Belder notifications@github.com wrote:

@indutnyhttps://github.com/indutny@indutny
https://github.com/indutny Interesting. What I found is that the number
of callbacks that node expects (stream._wriableState.pendingcb) doesn't
match what libuv thinks.


Reply to this email directly or view it on GitHub
#1155 (comment).

@piscisaureus

Copy link
Copy Markdown
Contributor

@indutny

This is fine, because they are queued in TLSWrap

It isn't, because this causes the socket to be destroyed before all writes are flushed (What happens is that pendingcb is 0 in node, but libuv still has two chunks of unwritten data).

@indutny

Copy link
Copy Markdown
MemberAuthor

Aah, well... This is a TLSWrap bug anyway, not StreamBase

On Saturday, March 14, 2015, Bert Belder notifications@github.com wrote:

@indutnyhttps://github.com/indutny

This is fine, because they are queued in TLSWrap

It isn't, because this causes the socket to be destroyed before all writes
are flushed (What happens is that pendingcb is 0 in node, but libuv still
has two chunks of unwritten data).


Reply to this email directly or view it on GitHub
#1155 (comment).

@indutny

Copy link
Copy Markdown
MemberAuthor

On a second thought, it is very interesting case. This imbalance is happening because .shutdown() request is performing an uv_write() on TLS socket. Looking into improving this.

@indutny

Copy link
Copy Markdown
MemberAuthor

@piscisaureus LGTY?

@piscisaureus

Copy link
Copy Markdown
Contributor

@indutny lgtm!

indutny added a commit that referenced this pull request Mar 15, 2015
Fix the `parallel/test-tls-over-http-tunnel.js` on Windows by
re-enabling the accidentally disabled `.writev()` method on TLSWrap.
It appears that there is some subtle issue with shutdown timing and it
manifests itself when the chunks are written in separate packets. This
leads to concurrent `shutdown`/`destroy`, which breaks the test.
PR-URL: #1155
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@indutny

Copy link
Copy Markdown
MemberAuthor

Landed in 4eb8810, thank you!

@indutnyindutny closed this Mar 15, 2015
@indutny
indutny deleted the fix/tls-over-http-on-windows branch March 15, 2015 04:09
@rvaggrvagg mentioned this pull request Mar 17, 2015
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@indutny@piscisaureus