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 event loop based on QNX ionotify #669

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Implement event loop based on QNX ionotify #669

wants to merge 41 commits into from

Conversation

sfod
Copy link
Contributor

@sfod sfod commented Aug 27, 2024

Implement CRT event loop based on QNX event mechanism.

  • Add a callback to the aws_io_handle struct. This callback is called by I/O operations to provide results to event loop. For backward compatibility, it is hidden by ifdef constructions.
  • Use ionotify, a special function provided by QNX to handle I/O events, to implement CRT event loop.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 39.21569% with 31 lines in your changes missing coverage. Please review.

Project coverage is 80.04%. Comparing base (c345d77) to head (5d9f58f).

Files with missing lines Patch % Lines
source/posix/pipe.c 18.75% 13 Missing ⚠️
source/posix/socket.c 44.44% 10 Missing ⚠️
source/s2n/s2n_tls_channel_handler.c 0.00% 4 Missing ⚠️
source/socket_channel_handler.c 69.23% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #669      +/-   ##
==========================================
- Coverage   80.39%   80.04%   -0.35%     
==========================================
  Files          28       28              
  Lines        5992     6048      +56     
==========================================
+ Hits         4817     4841      +24     
- Misses       1175     1207      +32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sfod sfod marked this pull request as ready for review August 27, 2024 22:48
@sfod sfod changed the title Provide I/O operation status back to event loop Provide I/O operation status back to event loop [DO NOT MERGE] Aug 27, 2024
@bretambrose
Copy link
Contributor

That's a lot of conditional compilation. Is that intended to be final?

@sfod
Copy link
Contributor Author

sfod commented Aug 29, 2024

Short answer: it's totally possible to completely remove all these compile-time conditions from existing sources.

Now, some details.

  • All changes in the source/posix/ dir can be easily moved to the new source/qnx/ dir. This way, we'll have two almost identical posix implementations - one with events with feedback, one with no feedback.

  • socket_channel_handler.c could also be split into two versions easily by moving it into source/something/ dir.

  • As for io.h. It's a public header, so it's trickier. Some cmake magic is definitely can be done to use different headers depending on the platform, but I didn't look into it yet.

To be honest, I want to keep all these checks, because otherwise every fix to posix implementation or socket_channel should be duplicated in the qnx implementation.

Apart from this, do you think it's a viable solution to add feedback functionality to I/O events?

include/aws/io/io.h Show resolved Hide resolved
source/posix/pipe.c Outdated Show resolved Hide resolved
source/posix/pipe.c Outdated Show resolved Hide resolved
source/posix/socket.c Outdated Show resolved Hide resolved
@sfod sfod changed the title Provide I/O operation status back to event loop [DO NOT MERGE] Implement event loop based on QNX ionotify Sep 16, 2024
@waahm7 waahm7 mentioned this pull request Sep 16, 2024
2 tasks
Copy link
Contributor

@bretambrose bretambrose left a comment

Choose a reason for hiding this comment

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

Some quick Initial feedback:

The conditional compilation should be removed from the headers and source. We dont have any backwards compatibility guarantees with the C libraries across different versions.

Additionally, I'm really concerned about the massive duplication between the qnx and posix folders from a maintainability standpoint.

source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
static int s_run(struct aws_event_loop *event_loop) {
struct ionotify_loop *ionotify_loop = event_loop->impl_data;

AWS_LOGF_INFO(AWS_LS_IO_EVENT_LOOP, "id=%p: Starting event-loop thread.", (void *)event_loop);
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do it here, but this reminds me that I think we need to start tagging event loop logging with event loop type (iocp, kqueue, epoll, gcd, inotify) for easier disambiguation, especially if some platforms might have multiple types available (mac eventually)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can actually introduce a new subject, AWS_LS_IO_EVENT_LOOP_IONOTIFY - "event-loop-ionotify". It won't take much effort. Unless you have something else in mind, then I'll leave it as is.

source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Outdated Show resolved Hide resolved
source/qnx/ionotify_event_loop.c Show resolved Hide resolved
AWS_LOGF_TRACE(AWS_LS_IO_EVENT_LOOP, "id=%p: fd is writable", (void *)event_loop);
event_mask |= AWS_IO_EVENT_TYPE_WRITABLE;
}
if (pulse->value.sival_int & _NOTIFY_COND_EXTEN) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Under what circumstances would this happen and what does it mean? All we seem to do is log it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this one is tough. I added a comment describing my interaction with this flag. I suggest we keep this log, maybe it'll reveal something.

@sfod sfod mentioned this pull request Oct 22, 2024
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