-
Notifications
You must be signed in to change notification settings - Fork 909
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
net: enable native golang linux networking #4498
base: dev
Are you sure you want to change the base?
Conversation
bbd514e
to
fdc75d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you test this?
To accept this into TinyGo, I'd like to see at least some tests. For example, you can create a new test like testdata/net.go (and add it to main_test.go for Linux only). It doesn't have to be exhaustive, but for example you can start a socket server (net.Listen
) in a goroutine and try to connect to it from the same program.
src/runtime/sync.go
Outdated
panic("todo: semacquire") | ||
// TODO the "net" pkg calls this, so panic() isn't an option. Right | ||
// now, just ignore the call. | ||
// panic("todo: semacquire") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this. There is a very good reason this panics: it is a reminder this should be implemented when it is used.
So either restore the panic, or actually implement semacquire
and semrelease
.
(It's probably easier to implement the internal/poll package instead in a way that works with TinyGo - but even that might be rather complex).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I am missing something here, but since tinygo at the moment only supports a single os thread, it would be sufficient to implement this using a simple mutex, right? Making this code panic will eventually reduce usability for this drastically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. All this API provides is a uint32
to store state. That's not enough to store a queue of waiting goroutines and the counter as would be needed for proper semaphore support. I don't think this can even be implemented in the current scheduler - at leat, not without some expensive data structures on the side. It is relatively easy to implement with a futex though, as I have in my threading PR: #4559
@aykevl I will add some basic tests. Since I already did some testing (I hope) it should be fairly easy to integrate this here. |
4ec411d
to
e1f18b2
Compare
65a36db
to
95a664b
Compare
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
if this does not work, do the folliwing steps: 1. remove net submodule 2. remove symlink in local ~/.cache/tinygo/goroot-<hash>/net 3. manual symlink yo local golang /usr/local/bin/src/net Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
Signed-off-by: leongross <[email protected]>
95a664b
to
81fd6ff
Compare
Signed-off-by: leongross <[email protected]>
81fd6ff
to
52be3d8
Compare
This is a rework of the plain linux network POC #4460. This PR adds adaptive switching of network packages depending on the
GOOS
.The CI fails due to unadjusted build tags for MacOs and WASm.
Successfully tested on
linux/amd64
.