(asys) API #8832

Aurel300 · 2019-09-20T18:08:40Z

This PR is just the Haxe part of the new asys APIs. This includes:

all types in asys - many functions were stubbed out an marked extern since the actual implementations will be added with target-specific asys PRs
some types in haxe:
- Error, ErrorType
some types in haxe.io:
- Duplex, FilePath, IDuplex, IReadable, IWritable, Readable, StreamTools, Transform, Writable

The types in asys will only be available on libuv-backed system targets (new define asys, pf_asys in OCaml).

Simn · 2019-09-21T05:48:49Z

As a general rule, nothing in std outside the asys package should refer to it. This is currently a problem for the following files in this PR:

haxe.Error: asys.uv.UVErrorType
haxe.ErrorType: asys.uv.UVErrorType
haxe.io.Duplex: import haxe.async.*
haxe.io.IReadable: import haxe.async.*
haxe.io.IWritable: import haxe.async.*
haxe.io.Readable: import haxe.async.*
haxe.io.Transform: haxe.async.*
haxe.io.Writable: haxe.async.*

By transitive property, it also applies to:

haxe.io.IDuplex: IReadable and IWritable
haxe.io.StreamTools: IReadable and IWritable

Simn · 2019-09-21T05:51:59Z

#8817 has this TODO:

For eval the API exposed to Haxe should be made closer to the one exposed by HashLink – less logic in OCaml, more in shared code

How does that affect this PR?

Aurel300 · 2019-09-21T08:04:29Z

How is importing haxe.async.* a problem? I guess it's just Defer (which can/should fallback to sys.Timer, and isn't used in most types anyway)?

How does that affect this PR?

That's a TODO for the eval-specifics only. The public API remains the same.

Simn · 2019-09-21T08:11:22Z

How is importing haxe.async.* a problem? I guess it's just Defer (which can/should fallback to sys.Timer, and isn't used in most types anyway)?

I don't know, I figured that imports were there for a reason. If they are unneeded we can just remove them.

That's a TODO for the eval-specifics only. The public API remains the same.

Ok, but what does "shared code" refer to then?

Simn · 2019-09-21T08:43:43Z

which can/should fallback to sys.Timer

Note that sys shouldn't be used from the haxe.io package either.

Aurel300 · 2019-09-21T09:34:19Z

I don't know, I figured that imports were there for a reason. If they are unneeded we can just remove them.

Yes, it's my bad. They are not unneeded, they are generally there for haxe.async.Callback.

Ok, but what does "shared code" refer to then?

"shared code" is e.g. in asys.net.Socket in #8817 – code that has logic in Haxe code, delegating the "real" native calls to a type from <target>.uv.*. For this to work the native types must have the same API, naturally.

Note that sys shouldn't be used from the haxe.io package either.

My bad, the fallback is haxe.Timer, not sys.Timer.

Simn · 2019-09-21T12:03:29Z

I'm trying to use Net.createServer and noticed two issues:

options is optional, but the code checks for options.listen which causes a null-access.
There's some confusion going on here with regards to the callbacks:
1. Net.createServer's callback is the Socket one which is invoked when a connection is made.
2. Server.listen's callback is the Void one which is invoked when the server is ready to listen.

As a consequence, I don't think createServer should call listen because you never get a chance to set up the second callback. Also, Server.listen's listener should become listeningSignal instead of connectionSignal.

This also makes it questionable if createServer's listener argument should be optional. I don't think creating a server without handling incoming connections makes sense.

RealyUniqueName · 2019-09-22T08:19:11Z

std/haxe/async/Callback.hx

+		var cb:Callback<NoData> = (err) -> trace("error!", err);
+		```
+	**/
+	@:from public static inline function fromErrorOnly(f:(error:Error) -> Void):Callback<NoData> {


I think this and fromErrorResult should not exist. It's a source for NPEs.

Could you give an example?

Sure

var cb:Callback<String> = function(error:Error) { // It's clear `null` is not expected here switch(error.type) {...} // null pointer error } successOp(cb);

var cb:Callback<String> = function(_, result:String) { // It's clear `null` is not expected here trace(result.length); // null pointer error } failOp(cb);

And it defeats null safety feature.

I don't think the first would compile, since fromErrorOnly only produces a Callback<NoData>.

I would argue the second is a user error. Subverting null safety sounds more serious though…

I don't think the first would compile, since fromErrorOnly only produces a Callback.

Data type does not matter in that example. It's still NPE even with Callback<NoData>. Maybe implementation should be changed to if(err != null) f(err).

I would argue the second is a user error.

That's a user error which could never happen if Null<> is respected by the API ;)

RealyUniqueName · 2019-09-22T08:28:33Z

std/asys/FileSystem.hx

+	asys.io.FileReadStream.FileReadStreamOptions;
+
+/**
+	This class provides methods for synchronous operations on files and


A bit weird to have a completely synchronous API in asys package

Well, we're not going to have multiple packages for this, and we cannot merge it with sys easily (more so due to asys.io.File incompatibilities with sys.io.File).

But

The types in asys will only be available on libuv-backed system targets

while asys.FileSystem and such are perfectly doable without libuv.

we're not going to have multiple packages for this

I think we should split it. Maybe io.sync and io.async or whatever better names?

RealyUniqueName · 2019-09-22T08:34:56Z

std/asys/FileSystem.hx

+
+	/**
+		Tests specific user permissions for the file specified by `path`. If the
+		check fails, throws an exception. `mode` is one or more `FileAccessMode`


Need to specify the type of exceptions.

RealyUniqueName · 2019-09-22T08:38:28Z

std/asys/FileSystem.hx

+		The result of this call should not be used in a condition before a call to
+		e.g. `open`, because this would introduce a race condition (the file could
+		be deleted after the `access` call, but before the `open` call). Instead,
+		the latter function should be called immediately and errors should be


Does this really guarantee avoidance of race conditions?
And in case one just needs to check access without immediately opening a file it forces a try..catch instead of a simple check.

And actually one can just handle exceptions on open to find out if required access is not permitted.

Related comment: #8832 (comment)

Yes, that's what the comment is trying to say, maybe I worded it badly. Basically, if you need a file, you should always try { var file = open(...); /* use file */ } catch ... rather than access, then open.

But what's the purpose of such access() then?

RealyUniqueName · 2019-09-22T08:40:19Z

std/asys/FileSystem.hx

+		`access` with the `mode`:
+
+		```haxe
+		FileAccessMode.Execute | FileAccessMode.Read


Is there a way to get which flag was rejected in this case?

No, uv basically wraps access(2), which will only error out with EACCES. The only way would be to check the flags individually.

RealyUniqueName · 2019-09-22T08:43:31Z

std/asys/FileSystem.hx

+		the latter function should be called immediately and errors should be
+		handled with a `try ... catch` block.
+	**/
+	static function access(path:FilePath, ?mode:FileAccessMode = FileAccessMode.Ok):Void;


I think mode should be mandatory (preferably) or at least FileAccessMode.Read as it's usually useless to know the file is there and to not know if any action on it could be performed.

How is this function supposed to be used? It returns Void, so I'm assuming it throws in case something is wrong. But then the documentation section about using the "result of this call" makes no sense.

Yes, it is a throwing function. I guess I thought of "result of this call" not as strictly the return value.

std/asys/FileSystem.hx

RealyUniqueName · 2019-09-22T08:51:50Z

std/asys/FileSystem.hx

+
+		TODO: `followSymLinks == false` is not implemented and will throw.
+	**/
+	static function chmod(path:FilePath, mode:FilePermissions, ?followSymLinks:Bool = true):Void;


Is this supposed to be recursive?

RealyUniqueName · 2019-09-22T08:52:22Z

std/asys/FileSystem.hx

+
+		TODO: `followSymLinks == false` is not implemented and will throw.
+	**/
+	static function chown(path:FilePath, uid:Int, gid:Int, ?followSymLinks:Bool = true):Void;


Is it recursive?

RealyUniqueName · 2019-09-22T08:53:37Z

std/asys/FileSystem.hx

+	static function chown(path:FilePath, uid:Int, gid:Int, ?followSymLinks:Bool = true):Void;
+
+	/**
+		Copies the file at `src` to `dest`. If `dest` exists, it is overwritten.


Can we change this to be explicit about rewriting an existing file? Maybe as an additional argument rewrite:Bool = false

The libuv call itself has no such argument, which means we would have to do something like:

if (rewrite || !FileSystem.exists(target)) copyFile(...); else throw "already exists";

Which introduces a race condition.

RealyUniqueName · 2019-09-22T08:58:26Z

std/asys/FileSystem.hx

+	Passing `null` as a path to any of the functions in this class will result
+	in unspecified behaviour.
+**/
+extern class FileSystem {


Let's have a method for recursive dir copy in std already, please? )

RealyUniqueName · 2019-09-22T09:01:10Z

std/asys/FileSystem.hx

+
+		The result of this call should not be used in a condition before a call to
+		e.g. `open`, because this would introduce a race condition (the file could
+		be deleted after the `exists` call, but before the `open` call). Instead,


I don't need to check file existance if I need to open it and handle exceptions anyway.

Again, yes, that's what the comment says. Do you understand it differently?

Yes, I read it (and in access doc too) as "instead of a check after exists, call open immediately".

RealyUniqueName · 2019-09-22T09:01:47Z

std/asys/FileSystem.hx

+	**/
+	static function exists(path:FilePath):Bool;
+
+	static function link(existingPath:FilePath, newPath:FilePath):Void;


Is this a symlink or hardlink?

Should it throw if newPath already exists?

RealyUniqueName · 2019-09-22T09:05:24Z

std/asys/FileSystem.hx

+
+	/**
+		Creates a unique temporary directory. `prefix` should be a path template
+		ending in six `X` characters, which will be replaced with random characters.


What's the behavior of prefix is not ending with XXXXXX?
Isn't it more convenient for a user to just provide whatever prefix which just gets appended with a random characters?

Yeah I find APIs with argument restrictions like that really stupid... I guess the C API is like that because you don't know string lengths in C, so the native function doesn't know how much to allocate for the applied template. We don't have to carry such awkwardness over into our API though.

RealyUniqueName · 2019-09-22T09:06:11Z

std/asys/FileSystem.hx

+
+		The generated directory needs to be manually deleted by the process.
+	**/
+	static function mkdtemp(prefix:FilePath):FilePath;


Maybe add ?mode:FilePermissions

std/asys/FileSystem.hx

RealyUniqueName · 2019-09-22T09:30:31Z

std/asys/FileSystem.hx

+
+	/**
+		Renames the file or directory located at `oldPath` to `newPath`. If a file
+		already exists at `newPath`, it is overwritten. If a directory already


I'd prefer overwrite:Bool = false instead of auto-overwriting.

Meh, that would be impractical I think. We could add the flag but it should default to true.

I'm ok with defult true, but the flag should be implemented.
Afaik filesystem paths are not lockable in general, so without such a flag we have no way to guarantee that some sensible data won't be overwritten on rename.

std/asys/Process.hx

RealyUniqueName · 2019-09-22T14:24:21Z

std/asys/Process.hx

+		Pipes created between the current (host) process and `this` (spawned)
+		process. The order corresponds to the `ProcessIO` specifiers in
+		`options.stdio` in `spawn`. This array can be used to access non-standard
+		pipes, i.e. file descriptors 3 and higher, as well as file descriptors 0-2


Maybe add public var pipes(get,never):Array<Pipe>?

What's the difference? That is basically what stdio is.

The difference is stdio also contains stderr, stdin, stdout. We have separate fields for those, but not for pipes.

stdio contains all the descriptors (This array can be used to access non-standard pipes, i.e. file descriptors 3 and higher, as well as file descriptors 0-2). It is indexed by fd, so it will naturally contain stderr, stdin, and stdout as 0, 1, and 2 (assuming they were created during spawn).

RealyUniqueName · 2019-09-22T14:54:34Z

std/asys/Timer.hx

@@ -0,0 +1,34 @@
+package asys;
+
+class Timer {


Let's add static function interval(f:()->Void, timeMs:Int):Timer which repeatedly executes f each timeMs milliseconds.

RealyUniqueName · 2019-09-22T15:00:10Z

std/asys/io/AsyncFile.hx

+	`callback:Callback<T>` argument, where `T` is the return type of the
+	synchronous method.
+
+	Errors are communicated through the callbacks or in some cases thrown


Is it possible to redirect all errors to callbacks so that one doesn't need to have two error-handling mechanisms for a single function call?

RealyUniqueName · 2019-09-22T15:03:23Z

Great work!
Finally Haxe is going to get a complete sys API :)

std/asys/io/IpcMessage.hx

std/asys/Process.hx

std/asys/net/Socket.hx

RealyUniqueName · 2019-09-27T15:03:55Z

std/asys/net/Socket.hx

+	uni-directional or bi-directional, depending on how it is created. Pipes can
+	be automatically created for spawned subprocesses with `Process.spawn`.
+**/
+class Socket extends Duplex {


Looking at a variety of "TCP only" and "IPC only" methods on options I think it would be better to have separate types like TcpSocket and IpcSoket

Maybe with a common ancestor.

RealyUniqueName · 2019-09-27T15:14:46Z

std/haxe/Error.hx

+	/**
+		Error type, usable for discerning error types with `switch` statements.
+	**/
+	public final type:ErrorType;


I don't like enum-based errors in Haxe. Any additional error type is a breaking change...

std/haxe/async/ArraySignal.hx

RealyUniqueName · 2019-09-27T15:24:06Z

std/haxe/async/Defer.hx

+		Convenience shortcut for `Timer.delay(f, 0)`.
+	**/
+	public static inline function nextTick(f:() -> Void):asys.Timer {
+		return asys.Timer.delay(f, 0);


I know this PR is about API, not implementations. But just be aware that haxe.Timer on java throws runtime errors if delay is 0ms. And small delays like 1ms don't guarantee the function won't be executed on the same tick.
See travis builds for the latest two commits here for example: https://github.com/haxe-utest/utest/commits/master?after=61cfb1d9997eaf18ebcd6c9f4a417a832e875116+0

RealyUniqueName · 2019-09-27T15:31:46Z

std/haxe/io/StreamTools.hx

+		`intermediate` is `null`, it is treated as an empty array and `input` is
+		connected directly to `output`.
+	**/
+	public static function pipeline(input:IReadable, ?intermediate:Array<IDuplex>, output:IWritable):Void {


Please, avoid optional args followed by mandatory args.
Discussion about removing skippable args feature keep coming. So let's not have an API which relies on it.

RealyUniqueName · 2019-09-27T15:36:47Z

This PR mentions "abstract base class which should not be instantiated" multiple times.
I think we need to replace "abstract" word with something else here. Because abstracts in Haxe is something different.

std/asys/Process.hx

Gama11 · 2019-09-28T14:30:07Z

std/asys/Process.hx

+	/**
+		Path to the working directory. Defaults to the current working directory if
+		not given.
+	**/
 	?cwd:FilePath,


The "short notation" of structs doesn't support doc comments.

RealyUniqueName · 2019-10-10T07:55:46Z

std/asys/FileSystem.hx

+	Passing `null` as a path to any of the functions in this class will result
+	in unspecified behaviour.
+**/
+extern class FileSystem {


Missing absolutePath method. E.g.: https://api.haxe.org/sys/FileSystem.html#absolutePath

Right, I forgot about it. Such a method should exist on the haxe.io.FilePath type, because it does not involve actually interacting with the OS.

I think there is no way to get the absolutePath without asking the OS where you are right now.

That is also true. There should still be a method that normalises a filepath, even without sys access – so it can resolve e.g. x/../y/../z/a/b/..///.. to z. If the path happens to be absolute to begin with, it should remain absolute.

What about: https://api.haxe.org/haxe/io/Path.html#normalize

@lublak Indeed, that is exactly what I meant ^^ I forgot it already exists in the APIs.

Aurel300 added 4 commits September 20, 2019 18:44

asys package

e896285

haxe package

616b89f

add asys define, forbid asys package on all targets for now

c36b72f

remove target-specific FilePath functions

e827e8f

Aurel300 added this to the Release 4.1 milestone Sep 20, 2019

Aurel300 mentioned this pull request Sep 20, 2019

asys #8817

Closed

4 tasks

RealyUniqueName reviewed Sep 22, 2019

View reviewed changes

std/asys/FileSystem.hx Show resolved Hide resolved

RealyUniqueName reviewed Sep 22, 2019

View reviewed changes

std/asys/FileSystem.hx Outdated Show resolved Hide resolved

RealyUniqueName reviewed Sep 22, 2019

View reviewed changes

std/asys/Process.hx Outdated Show resolved Hide resolved

RealyUniqueName reviewed Sep 22, 2019

View reviewed changes

RealyUniqueName reviewed Sep 23, 2019

View reviewed changes

std/asys/io/IpcMessage.hx Show resolved Hide resolved

RealyUniqueName reviewed Sep 23, 2019

View reviewed changes

std/asys/io/IpcMessage.hx Outdated Show resolved Hide resolved

typos

a001fc6

RealyUniqueName reviewed Sep 23, 2019

View reviewed changes

std/asys/Process.hx Show resolved Hide resolved

RealyUniqueName reviewed Sep 27, 2019

View reviewed changes

std/asys/net/Socket.hx Outdated Show resolved Hide resolved

RealyUniqueName reviewed Sep 27, 2019

View reviewed changes

std/asys/net/Socket.hx Outdated Show resolved Hide resolved

RealyUniqueName reviewed Sep 27, 2019

View reviewed changes

std/haxe/async/ArraySignal.hx Outdated Show resolved Hide resolved

RealyUniqueName reviewed Sep 27, 2019

View reviewed changes

Gama11 reviewed Sep 27, 2019

View reviewed changes

std/asys/Process.hx Outdated Show resolved Hide resolved

Aurel300 added 2 commits September 28, 2019 13:11

address API review

ca56786

swap connectFd arguments

2f02673

Gama11 reviewed Sep 28, 2019

View reviewed changes

RealyUniqueName reviewed Oct 10, 2019

View reviewed changes

Aurel300 mentioned this pull request Jan 27, 2020

(asys) Add libuv wrappers HaxeFoundation/hashlink#306

Draft

3 tasks

RealyUniqueName modified the milestones: Release 4.1, Release 4.2 Feb 17, 2020

RealyUniqueName modified the milestones: Release 4.2, Release 4.3 Dec 16, 2020

lublak mentioned this pull request Apr 22, 2021

Asynchronous IO API #9111

Open

12 tasks

Simn modified the milestones: Release 4.3, Later Mar 25, 2023

(asys) API #8832

Are you sure you want to change the base?

(asys) API #8832

Conversation

Aurel300 commented Sep 20, 2019

Simn commented Sep 21, 2019

Simn commented Sep 21, 2019

Aurel300 commented Sep 21, 2019

Simn commented Sep 21, 2019

Simn commented Sep 21, 2019

Aurel300 commented Sep 21, 2019

Simn commented Sep 21, 2019

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

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

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

RealyUniqueName Sep 22, 2019 • 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

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

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

RealyUniqueName commented Sep 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealyUniqueName Sep 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RealyUniqueName commented Sep 27, 2019

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

RealyUniqueName Sep 22, 2019 •

edited

Loading

RealyUniqueName Sep 27, 2019 •

edited

Loading