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

Hardening patchset #578

Merged
merged 25 commits into from
Feb 22, 2022

Conversation

ChrisCummins
Copy link
Contributor

@ChrisCummins ChrisCummins commented Feb 17, 2022

This patch set contains a handful of fixes for issues discovered while trying to track down #572:

  • Corrects a misuse of the boost::process API, fixing a bug wherein llvm-size child processes could become detached from the parent environment service and be left as zombie processes.
  • Disables TCP port reuse for all gRPC channels, as it has been reported to occasionally interfere with port autoselect (see Server port autoselect doesn't work reliably with SO_REUSEPORT on. grpc/grpc#10755).
  • Ensures that all gRPC client/server connection options in both Python and C++ use the same configuration options.
  • Fix a bug in which connection liveness was reported exactly the wrong way around (dead services were logged as alive and alive connections as dead).
  • Replaces get_system_includes() with get_system_library_flags(), a function that returns a list of -isystem flags, and a -L flag for macOS. This enables optimizing for runtime on macOS.
  • Holds the gRPC version back below 1.44 until I can diagnose the new error messages raised in the new version. See E0216 from backup_poller.cc or ev_epollex_linux.cc #572.
  • Adds a 300 second timeout to all tests when using the pytest test runner.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 17, 2022
Copy link
Contributor

@mostafaelhoushi mostafaelhoushi left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

Merging #578 (25c982d) into development (1d095e2) will increase coverage by 0.18%.
The diff coverage is 97.43%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #578      +/-   ##
===============================================
+ Coverage        88.09%   88.27%   +0.18%     
===============================================
  Files              114      114              
  Lines             6693     6705      +12     
===============================================
+ Hits              5896     5919      +23     
+ Misses             797      786      -11     
Impacted Files Coverage Δ
compiler_gym/envs/compiler_env.py 89.77% <ø> (-1.75%) ⬇️
compiler_gym/envs/llvm/__init__.py 100.00% <ø> (ø)
compiler_gym/envs/llvm/llvm_env.py 87.27% <ø> (ø)
compiler_gym/service/connection.py 82.95% <75.00%> (+6.96%) ⬆️
compiler_gym/envs/llvm/datasets/cbench.py 80.74% <100.00%> (+0.07%) ⬆️
compiler_gym/envs/llvm/datasets/csmith.py 86.84% <100.00%> (+0.17%) ⬆️
compiler_gym/envs/llvm/llvm_benchmark.py 93.43% <100.00%> (+2.73%) ⬆️
...ice/runtime/create_and_run_compiler_gym_service.py 100.00% <100.00%> (ø)
compiler_gym/views/observation_space_spec.py 85.71% <0.00%> (-3.58%) ⬇️
...loop_tool/service/loop_tool_compilation_session.py 88.43% <0.00%> (-2.73%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d095e2...25c982d. Read the comment docs.

This extends the logic for extracting #include search paths to also
extract the location of the -lSystem library from the host target.

This is to enable compiler binary support on macOS.
Fix a bug in which connection live was reported the wrong way
around (dead services were logged as alive and alive connections as
dead.

Also makes the representations more verbose to better explain what
type and state they are in.
boost::asio::io_service is only for backwards compatibility.
This corrects my use of the boost::process API whereby llvm-size child
processes could become detached from the parent environment service
and be left as zombies.

I found this issue will attempting to reproduce facebookresearch#572. I'm not sure if
this bug is relevant.
@ChrisCummins ChrisCummins changed the title Load test hardening patchset Hardening patchset Feb 22, 2022
@ChrisCummins ChrisCummins merged commit d70408f into facebookresearch:development Feb 22, 2022
@ChrisCummins ChrisCummins deleted the fix/flaky-rpc branch February 22, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants