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

Add non-blocking notes #180

Merged
merged 3 commits into from
Dec 9, 2023
Merged

Add non-blocking notes #180

merged 3 commits into from
Dec 9, 2023

Conversation

WeidiDeng
Copy link
Contributor

@WeidiDeng WeidiDeng commented Dec 9, 2023

I have done some local testing, it turns out if the file is in non-blocking mode, Getsize will return zero sizes without any error. Setting the file in block mode will allow the result to be correct.

Instead, we could document that we can manually set a file to be non-blocking mode using syscall.SetNonblock, this will allow deadlines to work. I have set a larger timeout so the tests will succeed every time. Possibly this is due to how golang internal poller works, but that's up to golang to fix it.

@WeidiDeng WeidiDeng changed the title Non blocking Add non-blocking notes Dec 9, 2023
@WeidiDeng
Copy link
Contributor Author

All tests passed in my workflow.

@WeidiDeng
Copy link
Contributor Author

I think -timeout parameter actually means the total time of running the test, not individual test. During my test, I found each test iteration took about 0.2 seconds, and 100 tests took 20 seconds. Definitely the polling is working as intended.

image

I did experience random get/set size errors when using non-blocking mode (manually set instead of using syscall.RawConn). And that's the reason I think we should document this case, let users configure by themselves.

@WeidiDeng
Copy link
Contributor Author

If I revert your changes in 3194d69. The tests do block forever. syscall.SetNonblock has no such problem if added to ioctl function.

@creack
Copy link
Owner

creack commented Dec 9, 2023

Thanks!

The last attempt to get that working tried to remove the uses of .Fd() (which sets blocking mode), but that resulted in various races. I'll keep testing in some of my projects with this version to be sure, but seem like it works as expected indeed.

@creack creack merged commit 03db72c into creack:master Dec 9, 2023
5 checks passed
renovate bot referenced this pull request in kairos-io/provider-kairos Aug 14, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/creack/pty](https://togithub.com/creack/pty) | `v1.1.21`
-> `v1.1.23` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fcreack%2fpty/v1.1.23?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fcreack%2fpty/v1.1.23?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fcreack%2fpty/v1.1.21/v1.1.23?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fcreack%2fpty/v1.1.21/v1.1.23?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>creack/pty (github.com/creack/pty)</summary>

### [`v1.1.23`](https://togithub.com/creack/pty/releases/tag/v1.1.23)

[Compare
Source](https://togithub.com/creack/pty/compare/v1.1.21...v1.1.23)

#### What's Changed

- Upgrade to go version 1.18.2 to fix multiple CVEs by
[@&#8203;pinaki124](https://togithub.com/pinaki124) in
[https://github.com/creack/pty/pull/154](https://togithub.com/creack/pty/pull/154)
- Tests cleanup by [@&#8203;creack](https://togithub.com/creack) in
[https://github.com/creack/pty/pull/173](https://togithub.com/creack/pty/pull/173)
- Revert [#&#8203;167](https://togithub.com/creack/pty/issues/167) to
avoid race on Linux. by [@&#8203;creack](https://togithub.com/creack) in
[https://github.com/creack/pty/pull/177](https://togithub.com/creack/pty/pull/177)
- Add non-blocking notes by
[@&#8203;WeidiDeng](https://togithub.com/WeidiDeng) in
[https://github.com/creack/pty/pull/180](https://togithub.com/creack/pty/pull/180)
- ztypes_openbsd\_32bit_int.go: remove arch list by
[@&#8203;n2vi](https://togithub.com/n2vi) in
[https://github.com/creack/pty/pull/189](https://togithub.com/creack/pty/pull/189)

#### New Contributors

- [@&#8203;pinaki124](https://togithub.com/pinaki124) made their first
contribution in
[https://github.com/creack/pty/pull/154](https://togithub.com/creack/pty/pull/154)
- [@&#8203;WeidiDeng](https://togithub.com/WeidiDeng) made their first
contribution in
[https://github.com/creack/pty/pull/180](https://togithub.com/creack/pty/pull/180)
- [@&#8203;n2vi](https://togithub.com/n2vi) made their first
contribution in
[https://github.com/creack/pty/pull/189](https://togithub.com/creack/pty/pull/189)

**Full Changelog**:
creack/pty@v1.1.20...v1.1.23

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 11pm every weekday,before 7am
every weekday,every weekend" in timezone Europe/Brussels, Automerge - At
any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/kairos-io/provider-kairos).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yNi4xIiwidXBkYXRlZEluVmVyIjoiMzguMjYuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants