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

http:make test destroyed-socket-write2.js robust #9270

Closed
wants to merge 1 commit into from
Closed

http:make test destroyed-socket-write2.js robust #9270

wants to merge 1 commit into from

Conversation

mhdawson
Copy link
Member

test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

modified:   test/simple/test-http-destroyed-socket-write2.js

@mhdawson
Copy link
Member Author

Initial discussion in this issue: #9234

@tjfontaine
Copy link

LGTM -- thanks for the excellent write up

@cjihrig
Copy link

cjihrig commented Feb 24, 2015

@mdawsonibm would you mind signing the commit with your full name? Also, can you reword the first line of the commit message to test: make destroyed-socket-write2.js more robust, or something similar.

test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

	modified:   test/simple/test-http-destroyed-socket-write2.js
@mhdawson
Copy link
Member Author

Ok, updated as requested. I assume "signing the commit with my full name" meant to change the author to "Michael Dawson" instead of "Michael"

mhdawson added a commit that referenced this pull request Feb 27, 2015
test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

PR-URL: #9270
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>
@cjihrig
Copy link

cjihrig commented Feb 27, 2015

Thanks! Landed in 3e40e12

@cjihrig cjihrig closed this Feb 27, 2015
mhdawson added a commit to ibmruntimes/node that referenced this pull request Mar 2, 2015
test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

PR-URL: nodejs#9270
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>
cjihrig pushed a commit to nodejs/node that referenced this pull request Mar 3, 2015
test/simple/test-http-destroyed-socket-write2.js validates
that you get an appropriate error when trying to write to
a request when the response on the other side has been destroyed.

The test uses http.request to get a request and then keeps writing
to it until either it hits 128 writes or gets the expected error.
Since the writes are asynchronous we see that the writes just end
up adding events to the event loop, which then later get processed
once the connection supporting the request is fully ready.

The test is timing dependent and if takes too long for the connection
to be made the limit of 128 writes is exceeded and the test fails.
The fact that the test allows a number of writes is probably to allow
some delay for the connection to be ready for writing.

On AIX, in the default configuration using the loopback interface
is slower and the test fails because the delay is such that many
more writes can be queued up before the connection takes place.
If we use the host ip instead of defaulting to the loopback then
the test passes.

The test needs to be made more robust to delays. Since each write
simply enqueues an additional write to the event queue there is
probably no point in doing the second write until the first has
completed. This patch schedules the next write when the first one
completes and allows the test to pass even if it takes longer for
the connection to be ready for writing

PR-URL: nodejs/node-v0.x-archive#9270
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Timothy J Fontaine <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants