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

test: loosen condition to detect infinite loop #1857

Closed
wants to merge 1 commit into from
Closed

test: loosen condition to detect infinite loop #1857

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

Under heavy load condition, this condition is strict that the test detects infinite loop.
So I have to loosen the condition more. currently I set the recursiveLimit is 100.
if over 100, throw Error that detects infinite loop.

this condition can detect original infinite loop problem and reduce the count of test failure like #1856 (comment)

@silverwind silverwind added the test Issues and PRs related to the tests. label Jun 1, 2015
@silverwind
Copy link
Contributor

Can that condition still be met with that kind of increase? I wonder when v8's Maximum call stack size exceeded triggers.

@yosuke-furukawa
Copy link
Member Author

$ node --v8-options
--stack_size (default size of stack region v8 is allowed to use (in kBytes))
        type: int  default: 984

984 kBytes is enough to call 100 recursive calls. I guess over 10000 loops, we should consider the Maximum call stack size exceeded warnings. (I tried 10000 loops, I cannot see the warnings).

My pull request decides the infinite loop when the recursive counts over 100 but received udp message count is less than 10.

I know this value is not perfect, I should loosen the value from limit + 1 (11) to X. it is not so easy to decide proper X for me.

If someone has suggestion, I will follow.

@silverwind
Copy link
Contributor

Well, I wouldn't put too much thought into it. LGTM if it increases CI reliabilty while still catching a real infinte loop.

@Fishrock123
Copy link
Member

LGTM, 100 depth recursion is pretty trivial for a test imo.

CI for good measure, if it looks good, land it. https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/752/

@yosuke-furukawa
Copy link
Member Author

some tests are failed, but those tests are unrelated to this PR.

@Fishrock123
Copy link
Member

Unrelated, yes, but slightly worrying. PR still LGTM to land.

Haven't seen either of these before:

not ok 146 - test-dgram-exclusive-implicit-bind.js

not ok 718 - test-tls-securepair-server.js
# ***server*** connection fd=undefined
# ***server*** i set it secure
# ***server*** connected+secure!
# ***server*** [object Object]
# ***server*** [object Object]
# ***server*** read bytes 7
# CONNECTED(00000003)
# ---
# Certificate chain
# 0 s:/C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
# i:/C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
# ---
# Server certificate
# -----BEGIN CERTIFICATE-----
# MIIDXTCCAkWgAwIBAgIJAMUSOvlaeyQHMA0GCSqGSIb3DQEBBQUAMEUxCzAJBgNV
# BAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBX
# aWRnaXRzIFB0eSBMdGQwHhcNMTAxMTE2MDkzMjQ5WhcNMTMxMTE1MDkzMjQ5WjBF
# MQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50
# ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIB
# CgKCAQEAz+LXZOjcQCJq3+ZKUFabj71oo/ex/XsBcFqtBThjjTw9CVEVwfPQQp4X
# wtPiB204vnYXwQ1/R2NdTQqCZu47l79LssL/u2a5Y9+0NEU3nQA5qdt+1FAE0c5o
# exPimXOrR3GWfKz7PmZ2O0117IeCUUXPG5U8umhDe/4mDF4ZNJiKc404WthquTqg
# S7rLQZHhZ6D0EnGnOkzlmxJMYPNHSOY1/6ivdNUUcC87awNEA3lgfhy25IyBK3QJ
# c+aYKNTbt70Lery3bu2wWLFGtmNiGlQTS4JsxImRsECTI727ObS7/FWAQsqW+COL
# 0Sa5BuMFrFIpjPrEe0ih7vRRbdmXRwIDAQABo1AwTjAdBgNVHQ4EFgQUDnV4d6mD
# tOnluLoCjkUHTX/n4agwHwYDVR0jBBgwFoAUDnV4d6mDtOnluLoCjkUHTX/n4agw
# DAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQUFAAOCAQEAFwV4MQfTo+qMv9JMiyno
# IEiqfOz4RgtmBqRnXUffcjS2dhc7/z+FPZnM79Kej8eLHoVfxCyWRHFlzm93vEdv
# wxOCrD13EDOi08OOZfxWyIlCa6Bg8cMAKqQzd2OvQOWqlRWBTThBJIhWflU33izX
# Qn5GdmYqhfpc+9ZHHGhvXNydtRQkdxVK2dZNzLBvBlLlRmtoClU7xm3A+/5dddeP
# AQHEPtyFlUw49VYtZ3ru6KqPms7MKvcRhYLsy9rwSfuuniMlx4d0bDR7TOkw0QQS
# A0N8MGQRQpzl4mw4jLzyM5d5QtuGBh2P6hPGa0YQxtI3RPT/p6ENzzBiAKXiSfzo
# xw==
# -----END CERTIFICATE-----
# subject=/C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
# issuer=/C=AU/ST=Some-State/O=Internet Widgits Pty Ltd
# ---
# No client certificate CA names sent
# ---
# SSL handshake has read 1197 bytes and written 684 bytes
# ---
# New, TLSv1/SSLv3, Cipher is AES256-SHA
# Server public key is 2048 bit
# Secure Renegotiation IS supported
# Compression: NONE
# Expansion: NONE
# No ALPN negotiated
# SSL-Session:
# Protocol  : TLSv1.2
# Cipher    : AES256-SHA
# Session-ID: B84425B6D5BFDF104CAAB0D8EF646D8E3F0E748D4572871853960F1E4B767A11
# Session-ID-ctx: 
# Master-Key: 0EE4488A175879164FE81A613EE706D9623CC14015863473E0BE7146B54C2D8BDD080EF98C7B2BBEA7139D7BEB0CD22E
# Key-Arg   : None
# PSK identity: None
# PSK identity hint: None
# SRP username: None
# TLS session ticket lifetime hint: 300 (seconds)
# TLS session ticket:
# 0000 - f3 09 69 49 4f b0 bf 2f-9c 2a 65 9c 72 7f 7b 16   ..iIO../.*e.r.{.
# 0010 - 89 4c d4 d2 fc 9e cd c2-85 6c a2 ae 10 9c 70 a1   .L.......l....p.
# 0020 - 91 7d bd eb 53 ce 9b 2a-d6 23 09 ec 0c a4 33 bd   .}..S..*.#....3.
# 0030 - 58 0e f7 43 47 5a c1 c8-88 ed 8a c7 50 0f fc 12   X..CGZ......P...
# 0040 - 28 87 67 35 87 67 6e 56-ae de e5 fa ac c5 3e ab   (.g5.gnV......>.
# 0050 - 94 6d 56 0a e0 92 b7 5a-ad d8 5a 58 1e 6f a1 e1   .mV....Z..ZX.o..
# 0060 - 3f 8e 72 59 e9 a8 14 0f-e0 20 4f ca 72 61 7b 3d   ?.rY..... O.ra{=
# 0070 - fb 8f 7a 74 9d 5e 90 77-d5 1c e8 b2 5f 13 46 47   ..zt.^.w...._.FG
# 0080 - e6 38 66 9d 7a bf 5c c0-0c 83 c2 5f 60 07 4c 34   .8f.z.\...._`.L4
# 0090 - de e8 5a 0d 4a 9f 87 50-01 af 4c ee 53 f9 bc d6   ..Z.J..P..L.S...
# 
# Start Time: 1433259883
# Timeout   : 300 (sec)
# Verify return code: 10 (certificate has expired)
# ---
# hello
# world

cc @nodejs/crypto, any idea what's up with the last one?

@silverwind
Copy link
Contributor

Cant read an error message out of that, but I guess we could update the test certs:

$ openssl x509 -enddate -noout -in test/fixtures/agent.crt
notAfter=Nov 15 09:32:49 2013 GMT

yosuke-furukawa added a commit that referenced this pull request Jun 2, 2015
PR-URL: #1857
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@yosuke-furukawa
Copy link
Member Author

landed d20f018
Thank you for your review.

@Fishrock123 Fishrock123 closed this Jun 2, 2015
@silverwind
Copy link
Contributor

Have to blame these random ARMv7 errors on the CI again, I can't reproduce this one in 100 runs on my pi2.

andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
PR-URL: nodejs/node#1857
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@rvagg rvagg mentioned this pull request Jun 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants