net: enable native golang linux networking #4498

leongross · 2024-10-04T14:02:23Z

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.

aykevl

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.

loader/goroot.go

.gitmodules

aykevl · 2024-10-10T10:47:38Z

src/runtime/sync.go

-	panic("todo: semacquire")
+	// TODO the "net" pkg calls this, so panic() isn't an option.  Right
+	// now, just ignore the call.
+	// panic("todo: semacquire")


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).

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.

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

I see that PR #4559 is still a wip. Maybe we should consider pulling out the futex functionality first to make some progress here?

@aykevl does this sound like a good approach to you?

src/os/exec.go

leongross · 2024-10-10T13:50:54Z

@aykevl I will add some basic tests. Since I already did some testing (I hope) it should be fairly easy to integrate this here.

src/syscall/forklock.go

aykevl · 2024-11-08T14:03:43Z

src/syscall/forklock.go

+// So for now, make our own stubbed-out ForkLock that doesn't use sync..
+
+type forklock struct{}
+
+func (f forklock) RLock()   {}
+func (f forklock) RUnlock() {}
+
+var ForkLock forklock


Don't. You're basically removing all safety here and breaking the API contract.

This again was stub code to get the poc working. Since your PR introduces threat safe mutexes we could replace it with

Suggested change

// So for now, make our own stubbed-out ForkLock that doesn't use sync..

type forklock struct{}

func (f forklock) RLock() {}

func (f forklock) RUnlock() {}

var ForkLock forklock

var ForkLock sync.RWMutex

aykevl · 2024-11-08T14:05:03Z

src/runtime/sync.go

+// "That is, don't think of these as semaphores.
+// Think of them as a way to implement sleep and wakeup
+// such that every sleep is paired with a single wakeup,
+// even if, due to races, the wakeup happens before the sleep."


Ah, that's useful to know. So they're not really semaphores but something like it.

https://cs.opensource.google/go/go/+/master:src/runtime/sema.go;l=12?q=semac&ss=go%2Fgo

aykevl · 2024-11-08T14:09:59Z

src/runtime/sync.go

+type semTable [semTabSize]struct {
+	root semaRoot
+	pad  [64 - unsafe.Sizeof(semaRoot{})]byte // only 64 x86_64, make this variable
+}
+
+func (t *semTable) rootFor(addr *uint32) *semaRoot {
+	return &t[(uintptr(unsafe.Pointer(addr))>>3)%semTabSize].root
+}


Can you explain what this is doing, and what it's for?

aykevl · 2024-11-08T14:10:39Z

src/runtime/sync.go

+	if cansemacquire(sema) {
+		return
+	}


This doesn't look like a complete implementation?
You need to wait for the other goroutine to do a semrelease. (This would be easy with multithreading and real OS futexes, but they don't exist yet).
Either you can busy loop with Gosched() (terrible idea) or put the waiting goroutine in a hashmap to be awoken by semrelease (slightly less terrible idea).

.gitmodules

GNUmakefile

aykevl · 2024-11-08T14:28:48Z

src/runtime/netpoll_generic.go

+	println("poll_runtime_pollReset not implemented", pd, mode)
+	return 1


This is also a bad idea. Either implement it, or exit with an error.

Returning 1 here indicates an error for this context. I added the proper definitions that should make it more clear.

aykevl · 2024-11-08T14:29:57Z

src/runtime/poll.go

-	panic("todo: runtime_pollServerInit")
+	// fmt.Printf("poll_runtime_pollServerInit not implemented, skipping panic\n")


If you put in debugging code, please remove it before sending in a PR. It makes the diff bigger and more work to review.
Generally, take a look at the PR diff and see if there's any part of it that is not necessary and can be removed (such as the changes to .gitmodules and the signal changes that are still part of this PR).
(Also, how do you know that you can just remove this panic?)

src/os/exec.go

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]>

aykevl · 2024-11-15T09:22:17Z

main_test.go

-		_, err := exec.LookPath(emulatorCommand)
-		if err != nil {
+		if _, err := exec.LookPath(emulatorCommand); err != nil {


Please don't make unrelated changes in the PR. While this is probably a good idea, it would be better to make a separate cleanup PR instead of putting everything in one huge PR.

leongross force-pushed the net-linux-nonlinux branch 7 times, most recently from bbd514e to fdc75d6 Compare October 8, 2024 09:35

leongross marked this pull request as ready for review October 8, 2024 09:47

aykevl reviewed Oct 10, 2024

View reviewed changes

leongross mentioned this pull request Oct 11, 2024

os: net.newUnixFile #4517

Merged

leongross force-pushed the net-linux-nonlinux branch 4 times, most recently from 4ec411d to e1f18b2 Compare October 28, 2024 09:57

leongross requested a review from aykevl November 4, 2024 10:06

leongross force-pushed the net-linux-nonlinux branch 3 times, most recently from 81fd6ff to 52be3d8 Compare November 4, 2024 16:32

aykevl requested changes Nov 8, 2024

View reviewed changes

leongross force-pushed the net-linux-nonlinux branch 8 times, most recently from 72cea16 to 182dd87 Compare November 11, 2024 12:04

leongross requested a review from aykevl November 13, 2024 08:23

link

ce1520d

Signed-off-by: leongross <[email protected]>

leongross added 11 commits November 13, 2024 09:25

linux support for net.Dial by stubbing out runtime polling

5f07a43

Signed-off-by: leongross <[email protected]>

remove print statements for cleaner execution

bdb93fe

Signed-off-by: leongross <[email protected]>

fix uncommited net exclusion

9c6020a

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]>

bring back old net

3215051

switch net via build tags

d698225

Signed-off-by: leongross <[email protected]>

adjust target os specific build tags

10221bb

chnage macos build

c40115c

add integration tests for net

d3abf3c

further restrict net package usage

f5ea847

Signed-off-by: leongross <[email protected]>

add preliminary semaphore implementation

5c8d4f6

Signed-off-by: leongross <[email protected]>

remove old forklock stub and replace it with sync.RWMutex

673d901

Signed-off-by: leongross <[email protected]>

leongross force-pushed the net-linux-nonlinux branch from e2e6576 to 1986937 Compare November 13, 2024 08:25

nit: update main_test.go

5f0f0aa

Signed-off-by: leongross <[email protected]>

leongross force-pushed the net-linux-nonlinux branch from 1986937 to 5f0f0aa Compare November 13, 2024 08:29

aykevl reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: enable native golang linux networking #4498

net: enable native golang linux networking #4498

leongross commented Oct 4, 2024 •

edited

Loading

aykevl left a comment

aykevl Oct 10, 2024 •

edited

Loading

leongross Oct 28, 2024 •

edited

Loading

aykevl Nov 4, 2024 •

edited

Loading

leongross Nov 6, 2024

leongross Nov 11, 2024

leongross commented Oct 10, 2024

aykevl Nov 8, 2024

leongross Nov 11, 2024 •

edited

Loading

aykevl Nov 8, 2024

leongross Nov 11, 2024

aykevl Nov 8, 2024

aykevl Nov 8, 2024

aykevl Nov 8, 2024

leongross Nov 11, 2024

aykevl Nov 8, 2024 •

edited

Loading

aykevl Nov 15, 2024

		println("poll_runtime_pollReset not implemented", pd, mode)
		return 1

		panic("todo: runtime_pollServerInit")
		// fmt.Printf("poll_runtime_pollServerInit not implemented, skipping panic\n")

net: enable native golang linux networking #4498

Are you sure you want to change the base?

net: enable native golang linux networking #4498

Conversation

leongross commented Oct 4, 2024 • edited Loading

aykevl left a comment

Choose a reason for hiding this comment

aykevl Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

leongross Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

aykevl Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leongross commented Oct 10, 2024

Choose a reason for hiding this comment

leongross Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leongross commented Oct 4, 2024 •

edited

Loading

aykevl Oct 10, 2024 •

edited

Loading

leongross Oct 28, 2024 •

edited

Loading

aykevl Nov 4, 2024 •

edited

Loading

leongross Nov 11, 2024 •

edited

Loading

aykevl Nov 8, 2024 •

edited

Loading