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

AF_UNIX missing in namespace tjs #512

Closed
KaruroChori opened this issue May 11, 2024 · 16 comments · Fixed by #530
Closed

AF_UNIX missing in namespace tjs #512

KaruroChori opened this issue May 11, 2024 · 16 comments · Fixed by #530

Comments

@KaruroChori
Copy link
Contributor

KaruroChori commented May 11, 2024

Unlike other socket related constants, this is missing.
In general it seems like the library is only focusing on the INET family.

Still, Unix Domain Sockets are working as expected once properly configured, so there is no specific reason to not support them I think.

Also, it would be nice to have js helpers to construct the addr structs.
At the moment we are forced to manually construct the Uint8Array, which works but it is a bit inconvenient.
In case, I can do it.

@lal12
Copy link
Contributor

lal12 commented May 11, 2024

I didn't implement a helper and the constant because it seemed more useful to use the normal streaming tjs socket for that. It was a generic implementation for any kind of sockets. E.g. I use it for raw sockets.

@lal12
Copy link
Contributor

lal12 commented May 13, 2024

What's your application using the PosixSocket btw?

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 13, 2024

I am using a file via UNIX Domain Sockets in streaming mode to share information with limited overhead between local processes and with external clients via a websockets bridge.
Or at least this is the objective :D. It kinda works, but I encountered #513

@saghul
Copy link
Owner

saghul commented May 13, 2024

Any reason not to use the builtin conect https://bettercallsaghul.com/txiki.js/api/functions/global.tjs.connect.html ? The pipe transport is unix sockets.

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 13, 2024

I am not sure I am following. The builtin connect is built around AF_INET, requiring an address and port. IPC with Unix Domain Sockets uses only a "filename" as address, The socket address type is just not the same.

To explain it better, I was forced to write something like

	//Hardcoded 1 since the symbol is not provided
	const sock = new PosixSocket(1, tjs.SOCK_STREAM, 0);
	const addr = new Uint8Array(110);
	addr[0] = 1;
	Array.from("../test-file").map(
		(letter, i) => (addr[2 + i] = letter.charCodeAt(0)),
	);

	sock.bind(addr);

It should be

	const sock = new PosixSocket(AF_UNIX, tjs.SOCK_STREAM, 0);
	const addr = somehelperfunction('../test-file')
	sock.bind(addr);

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 13, 2024

Ah I get it, it is called pipe internally in this implementation.
I had no idea that was an option, thanks

@saghul
Copy link
Owner

saghul commented May 13, 2024

It's a normal Unix domain socket, represented by a file.

Abstract sockets are not currently supported.

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 13, 2024

Yes, I was not planning on using them.
Just be clear, I assume there is no reason for them to work via posix-sockets.
You are just saying they are not supported by the higher level stream implementation you are providing, right?

@saghul
Copy link
Owner

saghul commented May 13, 2024

It would be nice if they work in the low level, I guess @lal12 didn't implement them because it wasn't necessary at the time.

In general the high level implementation is preferred, as it's cross-platform.

@KaruroChori
Copy link
Contributor Author

As far as I know, the abstract variant is a linux specific extension. So it is unlikely they can be made cross-platform, but I don't know enough about windows to tell.

Also, my understanding from the code is that pipes are assuming a stream-based connection. However Unix Domain Sockets would also allow for packet and sequence packet in place of SOCK_STREAM.

@saghul
Copy link
Owner

saghul commented May 13, 2024

As far as I know, the abstract variant is a linux specific extension. So it is unlikely they can be made cross-platform, but I don't know enough about windows to tell.

I mean Unix sockets / windows named pipes, not the abstract variant specifically.

Also, my understanding from the code is that pipes are assuming a stream-based connection. However Unix Domain Sockets would also allow for packet and sequence packet in place of SOCK_STREAM.

Correct, I missed to point that out. The libuv API which txiki.js wraps is only for streams.

@KaruroChori
Copy link
Contributor Author

KaruroChori commented May 13, 2024

I see. In principle, would you accept PR on what discussed so far?

More in general, is there any roadmap or policy about what is suitable for this project and what is not, aside from what reported in #418?

I am extending txiki quite a bit since I am adopting it as the runtime for an embedded application. A few of these features would make sense to be here in upstream, but without discussions enabled in this repo I would have to abuse pr or issues.

@saghul
Copy link
Owner

saghul commented May 13, 2024

I see. In principle, would you accept PR on what discussed so far?

Sure!

More in general, is there any roadmap or policy about what is suitable for this project and what is not, aside from what reported in #418?

No, I go by feeling :-)

I am extending txiki quite a bit since I am adopting it as the runtime for an embedded application. A few of these features would make sense to be here in upstream, but without discussions enabled in this repo I would have to abuse pr or issues.

Oh, right! I just enabled discussions, thanks for the heads up! Let;s discuss each individual change, thanks for your willingness to contribute!

@KaruroChori
Copy link
Contributor Author

No problem going by feeling, I just wanted to make sure I was not missing anything obvious as I usually do :D.
Great, thank you!

@saghul
Copy link
Owner

saghul commented May 31, 2024

I believe this one if fixed now, right?

@lal12
Copy link
Contributor

lal12 commented May 31, 2024

I believe this one if fixed now, right?

No, AF_UNIX isn't exposed if you mean that.
But I think it might be useful for opening DGRAM unix sockets or abstract unix sockets. I currently just define AF_UNIX myself. But exposing it is probably more useful. Will create a PR.

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 a pull request may close this issue.

3 participants