8000 tests: fix some potential gexpect hangs by lucab · Pull Request #3443 · rkt/rkt · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

tests: fix some potential gexpect hangs #3443

Merged
merged 1 commit into from
Dec 5, 2016
Merged
8000

tests: fix some potential gexpect hangs #3443

merged 1 commit into from
Dec 5, 2016

Conversation

lucab
Copy link
Member
@lucab lucab commented Dec 2, 2016

gexpect Wait() may let the child deadlock, blocked on a write. This replace some potentially dangerous occurrences with a waitOrFail() which drains child output.

Reference #3434
Fixes #3439 (comment)

@lucab lucab added this to the v1.21.0 milestone Dec 2, 2016
@lucab
Copy link
Member Author
lucab commented Dec 2, 2016

/cc @evverx

@lucab lucab mentioned this pull request Dec 2, 2016
Merged
@evverx
Copy link
Contributor
evverx commented Dec 3, 2016

@lucab, thanks!

for i in {1..50}; do echo $i; make functional-check GO_TEST_FUNC_ARGS='-run TestFetchFromFile' || break; done |& tee OUT

works!

BTW:
I'm reading #3434 (comment)
Is this a bug in child.Wait()? Or is this an expected behaviour?

@lucab
Copy link
Member Author
lucab commented Dec 5, 2016

I'm reading #3434 (comment)
Is this a bug in child.Wait()? Or is this an expected behaviour?

Unsure, but I think this is an architectural deficiency in gexpect itself. Most of gexpect assumes that the caller will handle output matching, either interactively or via the library. This applies also for Wait, even if it is never explicitly stated, resulting in this kind of subtle deadlocks.

@squeed
Copy link
Contributor
squeed commented Dec 5, 2016

LGTM, thanks!

s-urbaniak
s-urbaniak approved these changes Dec 5, 2016
View reviewed changes
Copy link
Contributor
@s-urbaniak s-urbaniak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@s-urbaniak s-urbaniak merged commit 9a4e5dd into rkt:master Dec 5, 2016
@evverx evverx mentioned this pull request Jan 26, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-urbaniak s-urbaniak s-urbaniak approved these changes

Assignees
No one assigned
Labels
area/developer tooling needs/review
Projects
None yet
Milestone
v1.21.0
Development

Successfully merging this pull request may close these issues.
4 participants
@lucab @evverx @squeed @s-urbaniak
0