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

CP-47001: stdext: quickcheck-style tests and select->epoll conversion #5402

Conversation

edwintorok
Copy link
Contributor

@edwintorok edwintorok commented Jan 29, 2024

This is a PR on top of #5378, we need to merge that first and then the history here will be a lot smaller.

Previous PR which had some comments:
xapi-project/stdext#80
xapi-project/stdext#83

I'll try to address those comments once I've done some more XenRT testing on this branch

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Additional details and impacted files
@@              Coverage Diff               @@
##           feature/perf   #5402     +/-   ##
==============================================
+ Coverage          51.8%   58.1%   +6.3%     
==============================================
  Files                19      16      -3     
  Lines              2586    2299    -287     
==============================================
- Hits               1341    1338      -3     
+ Misses             1245     961    -284     

see 5 files with indirect coverage changes

Flag Coverage Δ
python2.7 ?
python3.11 58.1% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@edwintorok
Copy link
Contributor Author

I fixed some bugs in the proxy code, and added some tests for it. Running some tests on this branch now.

@edwintorok edwintorok force-pushed the private/edvint/merge-stdext2-tests2 branch 2 times, most recently from 8346e67 to 2158c8e Compare February 1, 2024 10:08
@edwintorok
Copy link
Contributor Author

A XenRT BST of this was green, but given all the churn on master now I'll retest (and also do a bit more wider testing), I'm marking this as ready to review meanwhile though.

@edwintorok edwintorok marked this pull request as ready for review February 1, 2024 10:09
@edwintorok edwintorok changed the base branch from master to feature/epoll February 15, 2024 10:56
@edwintorok edwintorok changed the base branch from feature/epoll to feature/perf February 15, 2024 14:20
@edwintorok edwintorok force-pushed the private/edvint/merge-stdext2-tests2 branch 2 times, most recently from 6e6c4b3 to 62bff57 Compare February 15, 2024 17:40
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Why is the library placed in lib/ isntead of ocaml/libs? Does this PR mean that the Resources package and its unixfd are deprecated?

@edwintorok
Copy link
Contributor Author

Why is the library placed in lib/ isntead of ocaml/libs?

It got moved from a PR in the stdext repo, where the library was in lib. Git was smart enough to move most things across to ocaml/libs when cherry-picking, but not brand new files. Looks like I'll have to move those.

Does this PR mean that the Resources package and its unixfd are deprecated?

They solve different problems. Resources/safefd attempts to track open file descriptors and tries to prevent leaking them at compile time (by providing appropriate wrappers to close/move ownership), whereas this module is a runtime solution to prevent EBADF.
We could probably considerably simplify the resources/safefd library now that everything is in one repo, and we have this xapi-fd module (i.e. provide a thin wrapper on top of it).

I'll put this back into draft for now, so that I make the 'git mv' when I'm back from PTO.

@edwintorok edwintorok marked this pull request as draft March 28, 2024 18:38
This forces us to fully declare the dependencies of our code,
and not rely on libraries that are brought in only as transitive dependencies
of other libraries we happen to link to.

E.g. if our module A depends on library X, which itself depends on library Y,
then currently by linking X we also get Y linked and accessible from A
directly.
If code in module A uses both module X and Y *directly* then it needs to
declare a dependency on both when implicit transitive deps are off or it gets a
link failure (typically an error about a module or type being abstract).
If the code in module A only uses module X then no change is needed (X can
still use Y and the final executable will link both, it is just a question of
what is visible and callable from A directly).

This is especially useful when writing new code to get dependencies correct
from the beginning.

Signed-off-by: Edwin Török <[email protected]>
(cherry picked from commit 7203430)
…check

Bytecode builds for `http_lib` are disabled due to '(modes best)',
and that means that anything that depends on it must have it disabled too to avoid this warning.

Avoids these kinds of warnings:
```
File "_none_", line 1:
Error: Module `Buf_io' is unavailable (required by `Http_svr')
```

Signed-off-by: Edwin Török <[email protected]>
This will be a new library that will provide a more type-safe interface to file descriptor operations.
Useful on its own, but also for testing stdext.

Minimal dependencies, only Unix (and Alcotest for testing).

Signed-off-by: Edwin Török <[email protected]>
This will be a test framework providing QCheck generators and properties for
testing file descriptor operations.
It will try to generate:
* different kinds of file descriptors
* actual data written/read on the other end of pipes and socket pairs
* different speeds and delays on the other end to find buffering bugs
* file descriptors that are >1024 to find bugs with select

Signed-off-by: Edwin Török <[email protected]>
We are going to use type-level constraints a lot.
Try to future proof it by using the recommended compiler flag.
`ocamlc` says this about `-principal`:
> When using labelled arguments and/or polymorphic methods, this flag is required to
> ensure future versions of the compiler will be able to infer types correctly, even if internal algorithms change

Signed-off-by: Edwin Török <[email protected]>
This is not enabled by default (but bisect-ppx is nevertheless a build-time dependency)
Usage: `make coverage`

Signed-off-by: Edwin Török <[email protected]>
Lightweight wrapper using polymorphic variants to track read, write, and file kind properties on file descriptors.
We only track the property at the time the file descriptor was opened.

This prevents bugs like accidentally swapping the read and write ends of a pipe,
or attempting to run an operation on a file descriptor that would alway s fail (e.g. setting a socket timeout on a pipe)

Write tests using cram-style expect tests that the operations we expect to be forbidden by this type system
are actually forbidden.
The error messages may be compiler version dependent, so only run them on OCaml 4.14.1 for now.

Signed-off-by: Edwin Török <[email protected]>
Use the capabilities module to wrap most Unix operations needed in testing Unixext

Add a testsuite that checks that whenever the type says "never" the underlying file descriptor operation
would indeed raise an exception. This ensures that the type constraints we declare are actually correct.
The checks use unsafe operations that bypass the type layer.

Similarly check that operations that are accepted by the type system and marked as "always" in the type succeed.

Signed-off-by: Edwin Török <[email protected]>
It can be used to wrap read or write operations andobserve the data that is transferred,
and elapsed time.

It also provides 2 functions that create a file of a given kind.

We only test UNIX sockets, because socketpair doesn't support TCP sockets on Linux.

Signed-off-by: Edwin Török <[email protected]>
Uses 'xapi_fd_test'.

Signed-off-by: Edwin Török <[email protected]>
snwoods and others added 9 commits April 15, 2024 10:59
Signed-off-by: Steven Woods <[email protected]>
Signed-off-by: Edwin Török <[email protected]>
`Unix.write` would internally loop until the desired number of bytes are sent,
or `EAGAIN`/`EWOULDBLOCK`/another error is reached.
It cannot check for timeouts because it is not aware that we'd want one.

For pipes and sockets in non-blocking mode this wouldn't be a problem, but this function
is often called with block devices too.
However according to `pselect(3p)` it is a no-op on regular files:
"File descriptors associated with regular files shall always select true for ready to read, ready to write, and error conditions."
And the behaviour on block devices is left unspecified by POSIX, and `select(2)` on Linux doesn't document the behaviour either:
"The pselect() and select() functions shall support regular files, terminal and pseudo‐terminal devices, STREAMS‐based files, FIFOs, pipes, and sockets. The behavior of pselect() and select() on file descriptors that refer to other types of file is unspecified"

Although timeouts for a completely stuck block device cannot be implemented, we can still implement timeouts for a *slow* block device.
Use `Unix.single_{write,read}` instead which gives full control of the iteration to the caller.

The only way to interrupt stuck IO on a block device or regular file would be to use a separate process and `SIGKILL` it after a timeout (this is what `block_device_io` in XAPI does).

These approaches do not work:
* `alarm` or `setitimer` would only interrupt one thread in a multi-threaded program.
* `pthread_kill` can be used to send a signal to a particular thread, but that requires `EINTR` behaviour on syscalls to be enabled
* XAPI expects `SA_RESTART` semantics from syscalls, and would fail an assertion if it ever sees `EINTR` in some paths,
so although the syscall *would* get interrupted, it'd also immediately resume without giving the caller a chance to check for timeouts
* even if it'd work there are no bindings to `pthread_kill` in OCaml

Signed-off-by: Edwin Török <[email protected]>
'select' has a hardcoded limit of 1024 file descriptors.

Signed-off-by: Edwin Török <[email protected]>
It is too easy to misuse Unixext.time_limited_read because that one takesan absolute timestamp
as parameter, not a relative one.

Introduce a new function that takes a relative time as parameter, and doesn't loop.

Signed-off-by: Edwin Török <[email protected]>
@edwintorok edwintorok force-pushed the private/edvint/merge-stdext2-tests2 branch from 62bff57 to 3f9472c Compare April 15, 2024 10:02
@edwintorok
Copy link
Contributor Author

For now I've done a 'git mv' as the last step (I'll need to see whether I can use git filter-repo to move it to the correct place, a git rebase with git mv just results in conflicts).

@edwintorok edwintorok marked this pull request as ready for review April 15, 2024 10:03
@edwintorok edwintorok changed the title stdext: quickcheck-style tests and select->epoll conversion CP-47001: stdext: quickcheck-style tests and select->epoll conversion Apr 15, 2024
@edwintorok
Copy link
Contributor Author

edwintorok commented May 9, 2024

I fixed the conflicts with feature/perf, they were all in the dune files.

@edwintorok edwintorok merged commit e0eeb72 into xapi-project:feature/perf May 14, 2024
14 checks passed
Copy link

pytype_reporter extracted 47 problem reports from pytype output

.

You can check the results of the job here

@edwintorok edwintorok deleted the private/edvint/merge-stdext2-tests2 branch May 14, 2024 20:03
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.

4 participants