Skip to content

GitLab

  • Projects
  • Groups
  • Snippets
  • Help
    • Loading...
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
S slapos.core
  • Project overview
    • Project overview
    • Details
    • Activity
    • Releases
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Labels
    • Labels
  • Merge requests 26
    • Merge requests 26
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Operations
    • Operations
    • Environments
  • Analytics
    • Analytics
    • CI/CD
    • Repository
    • Value Stream
  • Members
    • Members
  • Activity
  • Graph
  • Jobs
  • Commits
Collapse sidebar
  • nexedi
  • slapos.core
  • Merge requests
  • !477

Merged
Created Jan 17, 2023 by Xavier Thompson@xavier_thompsonOwner

SlapPopen: Fix select-based timeout reads

  • Overview 8
  • Commits 2
  • Changes 2

Bug

Usage of slapos node instance shows that when a subprocess launched by SlapPopen deadlocks, the parent process also deadlocks (waiting in a blocking read) instead of timeouting, despite the select.poll-based approach intended to ensure that reads only occur when they would not block.

Quite possibly this is because the current implementation does not discriminate on poll events before reading, and the poll() call could happen to be woken up by an event that doesn't guarantee that the following read will be non-blocking (i.e. essentially an event other than "there is data to read" or "there is an error, the next read will fail (but not block!)").

Fix

These changes switch to using the higher-level selectors API instead of the low-level select.poll. Hopefully this should ensure that reads to stdout or stderr will only occur when guaranteed to be non-blocking. Another advantage is this implementation is more portable.

I was unable to devise a test that forced a deadlock with the previous implementation, so the theory about not discriminating on events is not proven, and it is not proven that this change would fix it. But at the very least this change makes the code more portable.

Alternative

Switching to a Popen.communicate(timeout=...) based approach should guarantee that timeouts are respected, but stdout would no longer be logged back in real time.

Edited Jan 17, 2023 by Xavier Thompson
Assignee
Assign to
Reviewer
Request review from
None
Milestone
None
Assign milestone
Time tracking
Source branch: fix/slappopen_poll_events_timeout
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7