10000 ports/unix: implement PEP 475 by dlech · Pull Request #5723 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

ports/unix: implement PEP 475 #5723

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

ports/unix: implement PEP 475 #5723

wants to merge 1 commit into from

Conversation

dlech
Copy link
Contributor
@dlech dlech commented Mar 5, 2020 •
edited
Loading

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 dlech force-pushed the pep-475 branch 2 times, most recently from 5f204d7 to b3bad6a Compare March 5, 2020 02:28
This was referenced Mar 5, 2020
Closed
Closed
@dlech dlech mentioned this pull request Mar 5, 2020
Open
50 tasks
@dlech dlech force-pushed the pep-475 branch 3 times, most recently from 99f377d to 130061d Compare March 5, 2020 03:14
@dlech
Copy link
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
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
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
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.

dpgeorge
dpgeorge reviewed Mar 20, 2020
View reviewed changes
ports/unix/mpconfigport.h Outdated
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 dlech mentioned this pull request Mar 21, 2020
Closed
@dlech
Copy link
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.

@dlech dlech mentioned this pull request Mar 25, 2020
Open
@dlech
ports/unix: Implement PEP 475
11cc3f6 
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
Contributor Author
< 8000 div> 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
@dlech dlech mentioned this pull request Mar 27, 2020
Closed
@amirgon amirgon mentioned this pull request Sep 3, 2021
Open
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Dec 29, 2021
@dhalbert
Merge pull request micropython#5723 from timhawes/traceback_fix
a526613 
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
Reviewers

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-unix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dlech @dpgeorge
0