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 io mode setter for POSIX #19120

Closed
wants to merge 16 commits into from

Conversation

quantimnot
Copy link
Contributor

No description provided.

lib/system/io.nim Outdated Show resolved Hide resolved
@quantimnot
Copy link
Contributor Author

Don't merge yet.

I'm writing a more comprehensive test that includes setting/unsetting the mode and exception handling.

@quantimnot quantimnot marked this pull request as draft November 11, 2021 17:09
@quantimnot
Copy link
Contributor Author

@elcritch
Does freertos have non-blocking io support?
From the CI results, I think it has fcntl and O_NONBLOCK, but my test case hangs, which is what it would do if the stdin fd wasn't non-blocking.

@quantimnot
Copy link
Contributor Author

quantimnot commented Nov 12, 2021

If I'm looking at the correct code here: https://github.com/FreeRTOS/FreeRTOS/blob/40c9e37d109fbfaa1e1940a97a600309bc6bd72a/FreeRTOS/Demo/Common/ethernet/lwip-1.4.0/src/api/sockets.c
then FreeRTOS fcntl impl is only concerned with sockets.

But if that is the case, why isn't the call returning a -1 value and then raising an exception?

Maybe each platform for FreeRTOS defines fcntl and their implementations return 0?
I don't know. I'm disabling the test for FreeRTOS.

@quantimnot
Copy link
Contributor Author

I want to disable the test for FreeRTOS, but Testament isn't working when I spec: disabled: "freertos".

I made #19137 to change that in Testament. Waiting on that.

@elcritch
Copy link
Contributor

Does freertos have non-blocking io support?
From the CI results, I think it has fcntl and O_NONBLOCK, but my test case hangs, which is what it would do if the stdin fd wasn't non-blocking.

It's not clear to me that it does. Or if it does that it'll work via POSIX. Also the tests for FreeRTOS were just meant for compile time checks as to properly test it'd need a QEMU setup.

@elcritch
Copy link
Contributor

But if that is the case, why isn't the call returning a -1 value and then raising an exception?

Maybe each platform for FreeRTOS defines fcntl and their implementations return 0? I don't know. I'm disabling the test for FreeRTOS.

That seems fair. The fcntl for on the RTOS'es don't always behave as expected by POSIX. Sometimes they accept a value but ignore it. Other times it can depend on whether you have another module compiled in.

Is O_NONBLOCK primarily for non-IP sockets?

@quantimnot
Copy link
Contributor Author

@elcritch

tests for FreeRTOS were just meant for compile time checks

Ok, maybe some other platform was hanging. It wasn't very clear to me.

Is O_NONBLOCK primarily for non-IP sockets?

I'm not sure, but I think it is for any fd type that can block, like pipes and sockets.

@quantimnot
Copy link
Contributor Author

quantimnot commented Nov 13, 2021

I don't see what's failing. Something about 'tests/vm/tslow_tables.nim c' timing out, but I don't see that in the logs. I'm not familiar with Azure Pipelines.

I ran the code on a Linux machine. It is hanging because the child proc isn't exiting or the io selector isn't signalling that it exited. I'll debug that when I get a chance over the next few days.

@elcritch
Copy link
Contributor

@quantimnot FYI there's a "View more details on Azure Pipelines" on the Azure pipelines logs that'll let you dive into the full error details.

setNonBlocking(stdin)
doAssert(endOfFile(stdin))
setNonBlocking(getOsFileHandle(f), nonBlocking)

Copy link
Contributor

Choose a reason for hiding this comment

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

Define for Windows too, but with a {.error: ....} (less confusing than getting a "Undeclared identifier" error

@elcritch
Copy link
Contributor

@quantimnot did you ever get this figured out? I'm up for helping figure out any of the RTOS conflicts.

@quantimnot
Copy link
Contributor Author

@elcritch Thank you for the offer to help. I've been away a few weeks while I've focused on some important matters.

I don't see any reasonable way to handle different RTOS ports having varying levels of POSIX compatibility, other than a build configuration phase that tests for compatibility and then sets a define flag. I could wrap the interface in a when defined hasPosixFcntl block, but we don't do that with any of the other POSIX interfaces, so that would be very inconsistent.

The current hangup with this PR is the test I made. A bug is making it hang on Linux. I haven't fixed it yet, because I got distracted on a POSIX FFI library that I've been experimenting with. I will get back to this as soon as I can.

I also want to change the interface after I have chatted with alaviss: alaviss/nim-sys#29

@elcritch
Copy link
Contributor

Reading through the PR, I think you could pretty readily keep the network and file logic separate at the cost of duplicating a few lines of code.

It seems like you could add these bits to setNonBlocking in native sockets:

when SupportIoctlInheritCtl:
  if c_ioctl(f, FIONBIO, unsafeAddr(opt)) == -1:

Then when SupportIoctlInheritCtl isn't defined the RTOS'es will try and execute the normal POSIX path. They should ignore it it's not available, or error out. Either no need to special RTOS handling at this point. Any RTOS tests currently should only compile the test, not run it.

@elcritch
Copy link
Contributor

Looking up FIONBIO I'm confused if using it is an improvement over the O_NONBLOCK? It's a bit of a weird corner but this stackoverflow makes it sound like O_NONBLOCK is more recent/more standard.

@stale
Copy link

stale bot commented Dec 26, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

Copy link
Contributor

This pull request is stale because it has been open for 1 year with no activity. Contribute more commits on the pull request and rebase it on the latest devel, or it will be closed in 30 days. Thank you for your contributions.

@github-actions github-actions bot added the stale Staled PR/issues; remove the label after fixing them label Dec 30, 2023
Copy link
Contributor

This pull request has been marked as stale and closed due to inactivity after 395 days.

@github-actions github-actions bot closed this Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants