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

Event Loop & Socket Type Multi-Support #692

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

Conversation

xiazhvera
Copy link
Contributor

@xiazhvera xiazhvera commented Nov 7, 2024

Issue #, if available:

Description of changes:

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 Nov 7, 2024

Codecov Report

Attention: Patch coverage is 66.24204% with 53 lines in your changes missing coverage. Please review.

Project coverage is 79.66%. Comparing base (fcb38c8) to head (ad5152c).

Files with missing lines Patch % Lines
source/event_loop.c 43.10% 33 Missing ⚠️
source/socket.c 75.00% 20 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   80.08%   79.66%   -0.43%     
==========================================
  Files          29       30       +1     
  Lines        6001     6122     +121     
==========================================
+ Hits         4806     4877      +71     
- Misses       1195     1245      +50     

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


🚨 Try these New Features:

@xiazhvera xiazhvera changed the title Event Loop Type Multi-Support Event Loop Type & Socket Multi-Support Nov 8, 2024
CMakeLists.txt Outdated Show resolved Hide resolved
@@ -172,6 +192,8 @@ struct aws_event_loop *aws_event_loop_new_default(struct aws_allocator *alloc, a
/**
* Creates an instance of the default event loop implementation for the current architecture and operating system using
* extendable options.
*
* Please note the event loop type defined in the options will be ignored.
*/
AWS_IO_API
struct aws_event_loop *aws_event_loop_new_default_with_options(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just eliminate all these alternate constructors and replace them with a single aws_event_loop_new(allocator, options)?

I mean ... they were already in a private header
or is that going to happen in some future PR?

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 eventually keep two constructors: aws_event_loop_new_default and aws_event_loop_new. aws_event_loop_new_default was used cross the aws-c-io tests to create default event loop. If we remove it, we probably need create help functions in all those test files, therefore I leave it there...

source/exponential_backoff_retry_strategy.c Outdated Show resolved Hide resolved
Comment on lines +132 to +133
AWS_ASSERT(false && "Invalid socket implementation on platform.");
return aws_socket_init_apple_nw_socket(socket, alloc, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this assert(false) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the point of this PR, apple network framework is not supported yet. Added the assertion to warn developer that we should never use this type so far.
I will update the comments to explan it.

source/socket.c Outdated Show resolved Hide resolved
Base automatically changed from EventLoopPublicApi to main November 12, 2024 17:45
@xiazhvera xiazhvera marked this pull request as ready for review November 12, 2024 23:59
* default.
*
* Default Event Loop Type
* Linux | AWS_EVENT_LOOP_EPOLL
Copy link
Contributor

Choose a reason for hiding this comment

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

Table isn't very well aligned

include/aws/io/private/event_loop_impl.h Show resolved Hide resolved
* `AWS_SOCKET_IMPL_PLATFORM_DEFAULT`, it will automatically use the platform’s default.
*
* PLATFORM DEFAULT SOCKET IMPLEMENTATION TYPE
* Linux | AWS_SOCKET_IMPL_POSIX
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting

include/aws/io/socket.h Outdated Show resolved Hide resolved
@@ -131,7 +131,8 @@ struct aws_event_loop_vtable s_kqueue_vtable = {
.is_on_callers_thread = s_is_event_thread,
};

struct aws_event_loop *aws_event_loop_new_default_with_options(
#ifdef AWS_ENABLE_KQUEUE
Copy link
Contributor

Choose a reason for hiding this comment

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

How would we be here if this wasn't defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If not defined AWS_ENABLE_KQUEUE, the following function should be defined. (in source/event_loop.c)

#ifndef AWS_ENABLE_KQUEUE
struct aws_event_loop *aws_event_loop_new_with_kqueue(
    struct aws_allocator *alloc,
    const struct aws_event_loop_options *options) {
    (void)alloc;
    (void)options;
    AWS_ASSERT(0);

    AWS_LOGF_DEBUG(AWS_LS_IO_EVENT_LOOP, "Kqueue is not supported on the platform");
    aws_raise_error(AWS_ERROR_PLATFORM_NOT_SUPPORTED);
    return NULL;
}
#endif // AWS_ENABLE_EPOLL

source/socket.c Outdated Show resolved Hide resolved
source/socket.c Outdated Show resolved Hide resolved
source/socket.c Outdated Show resolved Hide resolved
tests/event_loop_test.c Show resolved Hide resolved
source/windows/iocp/socket.c Outdated Show resolved Hide resolved
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