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

ports/unix: implement PEP 475 #5723

Closed
wants to merge 1 commit into from
Closed

Conversation

dlech
Copy link
Sponsor Contributor

@dlech dlech commented Mar 5, 2020

https://www.python.org/dev/peps/pep-0475/

This implements something similar to PEP 475 on the unix port.

There are a few differences from the CPython implementation:

  • Since we call mp_handle_pending(), addition functions could be called if MICROPY_ENABLE_SCHEDULER is enabled, not just signal handlers.
  • CPython only handles signal on the main thread, so other threads will raise InterruptedError instead of retrying. On MicroPython, mp_handle_pending() will currently raise exceptions on any thread.

@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 19, 2020

rebased on after #5769

@dpgeorge
Copy link
Member

Thanks for this, it's a good addition. I was just thinking if there could be a way to eliminate the duplication of code, all the very similar for-ever loops that keep going while there is EINTR. Possible ways to factor the code:

  1. using a (possibly inline) function that takes a system call function to keep calling while it gets EINTR.
  2. using a macro

Macros and inline functions won't help to reduce code size, but probably a macro is the only way to do it without getting to messy.

@dpgeorge
Copy link
Member

Some other points:

  • It might be worth considering a way to enable/disable this feature at compile time. Eg ports like esp32 and zephyr which provide POSIX-like APIs may want to use these POSIX modules, and they might not have this EINTR behaviour.
  • It will be hard to get full coverage testing for these additions...

@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 20, 2020

I've added a few commits for discussion. They can be squashed if all are acceptable.

I made a macro MP_HAL_RETRY_SYSCALL as suggested. I think it is OK, but it kind of hides a lot. For example...

    // not obvious bug
    MP_HAL_RETRY_SYSCALL(res, stat(mp_obj_str_get_str(path_in), &sb), mp_raise_OSError(err));

    // more obvious bug
    MP_THREAD_GIL_EXIT();
    size_t res = stat(mp_obj_str_get_str(path_in), &sb);
    MP_THREAD_GIL_ENTER();

There was quite a variety of cases to cover with a single macro. I was able to get all but one without getting too crazy.


I also made an intermediate commit with a MP_HAL_CONTINUE_IF_EINTR that covers a much smaller amount of code, but still covers the case of other ports wanting to reuse the posix code without EINTR. These ports could just define that macro as a nop. So if MP_HAL_RETRY_SYSCALL it too complicated, this could be a simpler option.

@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 20, 2020

It will be hard to get full coverage testing for these additions...

Yes, we would need to use something like ptrace to intercept the system calls. e.g. https://nullprogram.com/blog/2018/06/23/

@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 20, 2020

The unix coverage thread_stacksize1 test failure seems to be unrelated to this PR as I am getting the same failure locally without these changes.

@dpgeorge
Copy link
Member

Thanks for updating. I think the MP_HAL_RETRY_SYSCALL() is probably as good as it could get, but I agree it is hiding a lot of detail and will most likely be prone to bugs when using it. OTOH, having to rewrite the same code many times (if the macro wasn't used) could also lead to bugs.

MP_THREAD_GIL_ENTER(); \
(void)ret; \
} while (0)
#endif
Copy link
Member

Choose a reason for hiding this comment

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

removal of this is a nice clean-up, can be done in a separate commit/PR

@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 22, 2020

I went ahead and squashed the commits, keeping the MP_HAL_RETRY_SYSCALL macro. There is a finite list syscalls in PEP 475 that it applies to anyway, so not too many chances to mess it up anyway.

Also I split out the cleanup as suggested in #5782, so it would be best to merge that first before merging this PR.

https://www.python.org/dev/peps/pep-0475/

This implements something similar to PEP 475 on the unix port.

There are a few differences from the CPython implementation:
- Since we call mp_handle_pending(), addition functions could be called
  if MICROPY_ENABLE_SCHEDULER is enabled, not just signal handlers.
- CPython only handles signal on the main thread, so other threads will
  raise InterruptedError instead of retrying. On MicroPython,
  mp_handle_pending() will currently raise exceptions on any thread.

A new macro MP_HAL_RETRY_SYSCALL is introduced to reduce duplicated
code and ensure that all instances behave the same. This will also
allow other ports that use posix-like system calls to provide their
own implementation if needed.
@dlech
Copy link
Sponsor Contributor Author

dlech commented Mar 27, 2020

rebased to fix merge conflict

@dpgeorge
Copy link
Member

Ok, merged in 9418611

@dpgeorge dpgeorge closed this Mar 27, 2020
@dlech dlech deleted the pep-475 branch March 27, 2020 03:48
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Dec 29, 2021
traceback: fix for crash on non-native exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants