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

Implement module resource #1490

Merged
merged 12 commits into from
Jun 15, 2022
Merged

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Jun 3, 2022

Module resource is Posix-only. The implementation here is fairly complete as far as I know, except for a small incompatibility when attempting to increase a maximum limit while not being a superuser. CPython raises ValueError while this implementation raises OSError. I think CPython's choice is a mistake. A ValueError is supposed to indicate an invalid value, something that the caller should be able to prevent from happening with proper value checks and preconditions. For instance, trying to set a a pair of limits where current is higher than maximum is a legitimate ValueError. But to decide whether the caller is or is not allowed to raise the maximum is the domain of the OS. Some lightweight OS implementations may allow it in some circumstances or for some users. Therefore OSError is more appropriate, especially since the call already can raise OSError when the systems says "ni" for some reason.

But this is not the main reason I haven't implemented the CPython's behaviour in such case. In general, I prefer to implement all CPython's quirkiness, as unreasonable as it may be — for the sake of compatibility. In this case, however, I think it is not worth the effort, performance, and extra complexity. First, such situations should be rare. Second, to actually properly raise ValueError, the implementation would need to add another system call (at least); all this just to report the expected error type for a rare case. Finally, when I looked up how real life modules on PyPI handle such calls, in all cases I found they were catching both ValueError and OSError together, which is not surprising, given that the caller cannot predict which one might happen. (This by itself is another indication that the CPython's choice is unfortunate). So unless some real-life problem gets reported, I leave it as it is.

Still to do: make working on macOS.

@BCSharp BCSharp marked this pull request as ready for review June 10, 2022 16:44
# The .NET Foundation licenses this file to you under the Apache 2.0 License.
# See the LICENSE file in the project root for more information.

# tests fro module 'resource' (Posix only)
Copy link
Member Author

Choose a reason for hiding this comment

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

Typo 'fro' => 'for'

Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

I have any idea what this module does so if you're happy with it then so am I! 😄

def setUp(self):
self.RLIM_NLIMITS = 16 if is_linux else 9

@unittest.skipUnless(is_posix, "Posix-specific test")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can put the skipUnless on the class.

public static class PythonResourceModule {
public const string __doc__ = "Provides basic mechanisms for measuring and controlling system resources utilized by a program. Unix only.";

[Obsolete("Deprecated in favor of OSError.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does having the Obsolete tag produce any side effects in Python?

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't noticed any and I expect not. In all cases I saw resource used, the code was catching OSError rather than resource.error. It is here just for compatibility, but Python docs call it deprecated. There is a subtle difference between CPython and IronPython here, e.g.: in CPython, resource.error(2) creates OSError, but in IronPython it creates FileNotFoundError. The same applies to OSError(2) by the way. Not a big deal, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant side effects as in something like a DeprecationWarning. Not finding a Obsolete attribute on a property to, but clr.GetDynamicType appears to raise the warning. Not a big deal I guess...

in IronPython it creates FileNotFoundError

Ah interesting. Seems like a bug - only the 2 argument OSError constructor returns a subclass (e.g. OSError(2, '') becomes FileNotFoundError whereas OSError(2) should be OSError).


#endregion

[LightThrowing]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why chose to go LightThrowing throughout? Is it common to hit a throwing codepath?

Copy link
Member Author

@BCSharp BCSharp Jun 14, 2022

Choose a reason for hiding this comment

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

The code I saw was trying to increase the soft limit of number of open files, and ignoring the error if it failed. I suspect in many cases increasing limits is an optional optimization, so the try-ignore pattern may be common.

I was debating with myself a little whether I should only raise OSError as a light exception, or also ValueError. While there is a legitimate use case for light-ignoring an OSError here, a ValueError (usually) means that the caller has made an erroneous call and perhaps the call should fail loud and clear. But I don't expect this module to be used a lot from the C# side; I wrote it primarily because other packages depend on it (directly or indirectly). And given that the functions can already return a light exception and the confusing usage of ValueError by resource in CPython, I decided to return a light exception in all cases, to keep the door open for full CPython compatibility, if it ever becomes a need. But I could definitely reconsider this choice.

Comment on lines 355 to 356
public Mono.Unix.Native.Timeval ru_utime; // user CPU time used
public Mono.Unix.Native.Timeval ru_stime; // system CPU time used
Copy link
Member Author

Choose a reason for hiding this comment

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

I am not happy with using a type from Mono.Unix, here; well, I am not happy with Mono.Unix in general: it does not work correctly on macOS. Although there are no issues with Mono.Unix.Native.Timeval, I think that by avoiding using Mono.Unix altogether, the dependency could be removed at some point (at least on macOS, it seems to work OK on Linux).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think the dependency could be removed I'm fine with it - I just wanted to avoid having a bunch of platform specific P/Invokes which I may or may not be able to test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since Mono.Unix is proven not to work correctly in all cases, assuming that any call through this library will work as expected without any testing is asking for trouble. I myself got burned on this assumption a few times. So in practical terms, Mono.Unix must be considered untested (on macOS), and we need good test coverage regardless whether the call goes through the library or directly through P/Invoke. The advantage of P/Invoke is that it is readable/debuggable; I can check every signature and types used for correctness. On top of that, quite some of the missing functionality is not supported by Mono.Unix anyway (example: resource), and what is supported is usually also (and better) accessible through the regular .NET API. Further, in some cases that Mono does offer some additional and needed functionality and it happens to work, it tweaks the interface slightly to make it a little bit more convenient to use from C#. But this is not the best for IronPython: CPython's modules expose the C lib API at its raw low level, and accessing C lib directly though P/Invoke guarantees that the implementation does what CPython would do, no need to reverse-engineer Mono API. All in all, I think there will be only a small number of cases where Mono.Unix will be practical. This "small number" is not a scientific number, more my gut feeling, we'll see how it is after implementing all the missing modules. But everything is pointing in the direction that there will be "a bunch of platform specific P/Invokes" which will have to be covered by tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although there are no issues with Mono.Unix.Native.Timeval

I have spoken too soon. On Darwin, timeval is struct { long, int }. Mono.Unix.Native.Timeval is struct { long, long } (Linux layout).

@BCSharp
Copy link
Member Author

BCSharp commented Jun 15, 2022

OK, I did some reading of the source of Mono.Unix and that explained a lot. It turns out I was using the library incorrectly. Mono types, flags, error codes defined in Mono.Unix.Native are not (necessarily) the same as the corresponding native symbols of the underlying OS. Mono defines its own API that is the superset of all OS APIs, and maps them with a series of #ifdefs to the OS values/types. This is convenient for Mono applications, as they can simply use Mono types and constants with Mono calls and forget about the OS. This is less convenient for IronPython, because values passed from/to the Python user code are expected to be OS compliant. So to make a call to a Mono function, a user-supplied value has to be converted to the Mono language before the call, then Mono converts it back to the OS language and makes a P/Invoke. Similarly, in the opposite direction. The conversion functions in both directions are available in class Mono.Unix.Native.NativeConvert.

So I take back that Mono is badly tested on macOS. It is just that the conversions are (sometimes) missing. Mono "is working OK" on Linux because most constants and types are based on Linux. But since macOS uses sometimes different values and types, things were going wrong. With careful conversions, macOS should work fine too. I will probably review all places that IronPython makes a Mono call to check if correct conversions are in place.

Another consequence of the above is that Mono types and constants must not be used with direct P/Invoke calls. Since resource has to use P/Invoke, it was incorrect to use Timeval from Mono. The last committed version is correct.

@BCSharp BCSharp merged commit 99e337f into IronLanguages:master Jun 15, 2022
@BCSharp BCSharp deleted the resource_module branch June 15, 2022 22:13
slozier added a commit that referenced this pull request Jul 16, 2022
…1507)

* Fix a bug about wrong order of function arguments (#1495)

* Implement module resource (#1490)

* Implement resource.getrlimit

* Implement resource.setrlimit

* Implement resource.getpagesize

* Implement resource.getrusage

* Implement resources.prlimit

* Use RuntimeInformation.IsOSPlatform

* Additional tests

* Use light throwing

* Adapt to macOS

* Disable test on Windows

* Replace Mono.Unix calls and types

* Simplify tests

* Resolve failure with jedi module (#1496)

* Resolve failure with jedi module

* Fix CA1853

* Fix test failure

* Add test

* Use P/Invoke for os.strerror (#1500)

* nt.strerror_r

* Eliminate switch on PythonErrorNumber

* Isolate Mono.Unix

* Add a few tests

* Implement make target "purge" (#1503)

* Support module errno on Linux and macOS (#1502)

* Generate platform-specific errno codes

* Generate platform-specific errno symbols

* Add test_errno

* Support error symbol aliases

* Support error symbol aliases on Linux

* Support error symbol aliases on Darwin

* Include Posix aliases in Windows aliases

* Stable errno order

* Let _socket use OS-native error codes

* Make tests passing on all platforms

* More Mono codes remapped

* Complete mapping of Mono codes

Co-authored-by: Jingxuan He <LostBenjamin@users.noreply.github.com>
Co-authored-by: slozier <slozier@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.

None yet

2 participants