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

80-test_cmp_http.t: Prevent hangs on server not launching/behaving correctly, Improve the way the test server is launched and killed #15642

Closed
wants to merge 3 commits into from

Conversation

DDvO
Copy link
Contributor

@DDvO DDvO commented Jun 7, 2021

This is a follow-up of #15580, picking up loose ends discussed there.

  • tests are added or updated

@DDvO DDvO added the approval: otc review pending This pull request needs review by an OTC member label Jun 7, 2021
@richsalz
Copy link
Contributor

richsalz commented Jun 7, 2021

Maybe have the server write it's process ID into a file in the test-runs directory.

I am surprised that this is so complicated, given that the TLSProxy etc do not have these problems.

@slontis
Copy link
Member

slontis commented Jun 7, 2021

This appears to now cause a few more infinite loops..

@DDvO
Copy link
Contributor Author

DDvO commented Jun 7, 2021

Maybe have the server write it's process ID into a file in the test-runs directory.

That's a good thought, thank you - I now let the server simply emit its PID along with the ACCEPT string.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 7, 2021

This appears to now cause a few more infinite loops..

These are gone now :)

@levitte
Copy link
Member

levitte commented Jun 8, 2021

I am surprised that this is so complicated, given that the TLSProxy etc do not have these problems.

TLSProxy doesn't start the subprocess with an open of a pipe, but with a fork, and control is therefore different. That came with some pains as well, just different pains.

It's also possible that using perl's IPC modules directly would be better in the longer run. I don't know, I've never done that myself, but that might be something worth looking into at some point. (also gotta be careful that the modules used are perl's core modules since version n5.10)

@levitte
Copy link
Member

levitte commented Jun 8, 2021

I wonder whether you could add _RLD_LIB_PATH as:

local $ENV{_RLD_LIB_PATH} = $dir;

to account for how NonStop accounts for paths. I currently set this explicitly in the outer shell when testing.

We already have that in util/shlib_wrap.sh, which is being used by util/wrap.pl on Unix family platforms...

@DDvO
Copy link
Contributor Author

DDvO commented Jun 8, 2021

I wonder whether you could add _RLD_LIB_PATH as:

local $ENV{_RLD_LIB_PATH} = $dir;

to account for how NonStop accounts for paths. I currently set this explicitly in the outer shell when testing.

We already have that in util/shlib_wrap.sh, which is being used by util/wrap.pl on Unix family platforms...

NIce that this is now done automatically by calling cmdstr().

@DDvO
Copy link
Contributor Author

DDvO commented Jun 8, 2021

I think this PR is good state now.

@paulidale paulidale added approval: done This pull request has the required number of approvals branch: master Merge to master branch and removed approval: otc review pending This pull request needs review by an OTC member labels Jun 8, 2021
@DDvO DDvO changed the title 80-test_cmp_http.t: Prevent hangs on server not launching/behaving correctly, Improve setting of {,DY}LD_LIBRARY_PATH 80-test_cmp_http.t: Prevent hangs on server not launching/behaving correctly, Improve the way the test server is launched and killed Jun 8, 2021
@levitte
Copy link
Member

levitte commented Jun 8, 2021

Please rebase this on top of a fresh master. I would say that the approvals stand

levitte and others added 3 commits June 9, 2021 07:45
rel2abs() doesn't clean the path well enough, which may lead to odd
results when calculating new paths.  abs_path() works better for this
sort of thing.
@DDvO
Copy link
Contributor Author

DDvO commented Jun 9, 2021

Please rebase this on top of a fresh master. I would say that the approvals stand

Done (though there is not even a real merge conflict with commit 8e2b2336fb).

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@levitte
Copy link
Member

levitte commented Jun 9, 2021

Done (though there is not even a real merge conflict with commit 8e2b233).

For some reason, that commit hadn't made its way to github... now it has.

@DDvO
Copy link
Contributor Author

DDvO commented Jun 9, 2021

I think I just referred to an outdated commit ID, which must have changed due to rebasing - sorry for the confusion.

@levitte
Copy link
Member

levitte commented Jun 9, 2021

I think I just referred to an outdated commit ID, which must have changed due to rebasing - sorry for the confusion.

No, you didn't. That commit hadn't travelled from OpenSSL's main repository (which isn't on github) to github (openssl/openssl is just a mirror). I had to push that manually.

openssl-machine pushed a commit that referenced this pull request Jun 9, 2021
…g/behaving correctly

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15642)
openssl-machine pushed a commit that referenced this pull request Jun 9, 2021
…illed

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from #15642)
@DDvO
Copy link
Contributor Author

DDvO commented Jun 9, 2021

Merged - thanks

@DDvO DDvO closed this Jun 9, 2021
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
…g/behaving correctly

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15642)
devnexen pushed a commit to devnexen/openssl that referenced this pull request Jul 7, 2021
…illed

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#15642)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants