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

sockets refactoring #329

Merged
merged 2 commits into from
Jan 4, 2024
Merged

sockets refactoring #329

merged 2 commits into from
Jan 4, 2024

Conversation

guybedford
Copy link
Collaborator

Work-in-progress refactoring of the sockets implementation. Lots of small things, removing unnecessary steps where possible given that this is system-level code. Refactors include:

  • Simplifying the entrypoint structure to avoid nested function calls.
  • Unifying on the network capability system instead of making the main sockets API instanceable, since the network object captures the capability customizations without needing that.
  • Moving from DNS resolve to DNS lookup so that we are directly calling getaddrinfo with associated system hooks and caching, as I believe is expected. In addition fixed up the error handling and we also now return both IPv4 and IPv6 addresses and the ip name test is fully passing.
  • Always make family lower case, avoiding the need for explicit toLowerCase() elsewhere. Also moving away from toLocaleLowerCase() to not invoke the engine ICU in systems code.
  • Added ./sockets/*: null exports mapping to ensure the private modules are private.
  • Removed unnecessary ids in both sockets and the other APIs now that we are fully on the private ID pattern. Originally this was for debugging, but it hasn't been used that much so it seems fine to remove now.
  • Ensured that all ioCall invocations always provide three arguments to ensure engine monomorphism
  • Removed unnecessary Promise.resolve wrapping.
  • Updating the asserts to be direct errors instead of indirect calls.
  • Unified on private fields instead of using symbols for private slots in the sockets objects.
  • Inlining some private functions when only used once.

And then I removed the use of auto bind in UDP socket options application, as I don't think this is correct, the two udp options tests are now failing.

To resolve this, I plan to add the following to this PR today and tomorrow:

  • Refactoring the state on the tcp and udp socket classes, to use a singular state integer in all state checks.
  • Fix the use of the socket options setting APIs such that when in an earlier state, they cache and then apply immediately on bind, based on checking the above state integer to know which path to follow.

Once landed that will leave us with just the sample applications failing, at which point it should just be about debugging the final end-to-end implementation and less major refactoring.

@guybedford
Copy link
Collaborator Author

@manekinekko alternatively, to not block progress we could merge this without the remaining work, so that there is less churn in the mean time. Let me know if that might be preferable for you - this could be landed as-is.

@manekinekko
Copy link
Collaborator

I'm happy to land this PR as is. My only concern is the DNS lookup implementation. My understanding is that to avoid using getaddrinfo as it's mentioned in the specs https://github.com/WebAssembly/wasi-sockets#why-not-getaddrinfo. Did I understand that correctly?

@guybedford
Copy link
Collaborator Author

@manekinekko my interpretation of that is that getaddrinfo is not implemented verbatim to instead provide a more secure and constrained interface, but that that does not preclude implementations from using getaddrinfo as the underlying system implementation call, provided it is constrained to not use the features listed.

@guybedford guybedford changed the title WIP sockets refactoring sockets refactoring Jan 4, 2024
@guybedford guybedford marked this pull request as ready for review January 4, 2024 20:01
@guybedford guybedford merged commit 05b016b into main Jan 4, 2024
8 checks passed
@guybedford guybedford deleted the sockets-refactor branch January 4, 2024 20:01
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.

2 participants