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

Fix wasi-sockets tests #2389

Merged

Conversation

loganek
Copy link
Collaborator

@loganek loganek commented Jul 25, 2023

Tests were failing because the right permissions were not provided to iwasm.
Also, test failures didn't trigger build failure due to typo - also fixed in this change

In addition to that, I fixed a few issues with the test itself:

  • the server_init_complete was not reset early enough causing the client to occasionally assume the server started even though it didn't yet.
  • set SO_REUSEADDR on the server socket so the port can be reused shortly after closing the previous socket
  • defined receive-send-receive sequence from server to make sure server is alive at the time of sending messages from client

@loganek loganek force-pushed the loganek/wasi-sockets-tests-fix branch from fa7ad7b to 4aa9954 Compare July 25, 2023 14:22
@loganek loganek marked this pull request as draft July 25, 2023 14:22
@loganek loganek force-pushed the loganek/wasi-sockets-tests-fix branch 7 times, most recently from e7338ee to 53d28d1 Compare July 26, 2023 10:56
@loganek loganek marked this pull request as ready for review July 26, 2023 11:17
struct sockaddr_in6 addr_ipv6;
} addr;
typedef struct {
struct sockaddr_storage addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is it coming from? Is it still a union?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The spec doesn't define whether it's an union or not - usually it's a large enough array + common fields (address family) + some padding. sockaddr_storage is a structure that's guaranteed to be big enough to contain all the address types supported (see https://man7.org/linux/man-pages/man7/ipv6.7.html).

assert(pthread_join(server_thread, NULL) == 0);
assert(pthread_join(client_thread, NULL) == 0);

assert(pthread_mutex_destroy(&mut) == 0);
assert(pthread_cond_destroy(&cond) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it destroyed somewhere else? Looks like a leak

Copy link
Collaborator Author

@loganek loganek Jul 26, 2023

Choose a reason for hiding this comment

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

Calling pthread_mutex_destroy on statically initialized mutex is not needed; from the The Linux Programming Interface book:

It is not necessary to call pthread_mutex_destroy() on a mutex that was statically initialized using PTHREAD_MUTEX_INITIALIZER.

I think the same applies to pthread_cond_destroy

@loganek loganek requested a review from g0djan July 26, 2023 22:51
pthread_mutex_unlock(&mut);
pthread_cond_signal(&cond);
Copy link
Contributor

Choose a reason for hiding this comment

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

How about put pthread_cond_signal before pthread_mutex_unlock? I remember it is generally used by that way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, can move it, although I don't think that really matters.

Tests were failing because the right permissions were not provided to iwasm.
Also, test failures didn't trigger build failure due to typo - also fixed in this change

In addition to that, I fixed a few issues with the test itself:
* the `server_init_complete` was not reset early enough causing the client to occasionally assume the server started even though it didn't yet.
* set `SO_REUSEADDR` on the server socket so the port can be reused shortly after closing the previous socket
* defined receive-send-receive sequence from server to make sure server is alive at the time of sending messages from client
@loganek loganek force-pushed the loganek/wasi-sockets-tests-fix branch from 53d28d1 to 56e9594 Compare July 27, 2023 10:33
Copy link
Contributor

@wenyongh wenyongh left a comment

Choose a reason for hiding this comment

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

LGTM

@wenyongh
Copy link
Contributor

The PR #2364 is waiting for this PR, let's merge this PR first. If there is other issue found,
we can create another PR to fix it.

@wenyongh wenyongh merged commit 151600f into bytecodealliance:main Jul 30, 2023
victoryang00 pushed a commit to victoryang00/wamr-aot-gc-checkpoint-restore that referenced this pull request May 27, 2024
Tests were failing because the right permissions were not provided to iwasm.
Also, test failures didn't trigger build failure due to typo - also fixed in this change.

In addition to that, this PR fixes a few issues with the test itself:
* the `server_init_complete` was not reset early enough causing the client to occasionally
  assume the server started even though it didn't yet
* set `SO_REUSEADDR` on the server socket so the port can be reused shortly after
  closing the previous socket
* defined receive-send-receive sequence from server to make sure server is alive at the
  time of sending message
@loganek loganek deleted the loganek/wasi-sockets-tests-fix branch June 10, 2024 12:47
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.

3 participants