Async callbacks

[kenthinson]

Let me know if this is ok. I can make changes if you see other things that should be done differently.

[kareman]

Hi, this is excellent work and a really useful addition :). Better handling of asynchronous commands is definitely something SwiftShell needs.

Some thoughts:

• There are too many "e"s in "Handeler", it should be “Handler”.

• I completely agree that ShellError.errorcode should be an Int, it is already in my list of things that should be fixed in a future version. The problem is this is a backwards incompatible change, meaning current scripts and applications that use SwiftShell 2 might no longer compile if we make this change now. If we follow semantic versioning this change will have to wait for SwiftShell 3. This is a small change which will affect very few people, if any, so I'm quite possibly too cautious here but in any case this change would be better off in a separate pull request.

  BTW, adding the completion handler to runAsync is backwards compatible because we are adding a new parameter with a default value so existing code will still compile. We just have to release it as SwiftShell 2.1.

• I think the output handler and error handler should be moved to ReadableStream. That way, instead of having to provide 3 closures in one function call we can do this instead:

  let command = runAsync("command") {
    // do something when it finishes
  }
  command.stdout.onOutput = { stream in
    // "stream" is of type ReadableStream
  }
  command.stderror.onOutput = { stream in
    // "stream" is of type ReadableStream
  }

  This way the output handlers can be used on any ReadableStream, including main.stdin, which can be very useful.

  BTW I'm not sure about the name "onOutput", what do you think? Especially main.stdin.onOutput looks weird, like we're getting output from the input. Which I guess we kind of are, but it looks self-contradictory.

Oh and I like the stop method, very useful.

[kenthinson]

Sweet. I’ll work on it and get back to you. :)

On Apr 25, 2016, at 5:38 AM, Kare Morstol notifications@github.com wrote:

Hi, this is excellent work and a really useful addition. Better handling of asynchronous commands is definitely something SwiftShell needs.

Some thoughts:

There are too many "e"s in "Handeler", it should be “Handler”. I completely agree that ShellError.errorcode should be an Int, it is already in my list of things that should be fixed in a future version. The problem is this is a backwards incompatible change, meaning current scripts and applications that use SwiftShell 2 might no longer compile if we make this change now. If we follow semantic versioning http://semver.org/ this change will have to wait for SwiftShell 3. This is a small change which will affect very few people, if any, so I'm quite possibly too cautious here but in any case this change would be better off in a separate pull request.

BTW, adding the completion handler to runAsync is backwards compatible because we are adding a new parameter with a default value so existing code will still compile. We just have to release it as SwiftShell 2.1.

I think the output handler and error handler should be moved to ReadableStream. That way, instead of having to provide 3 closures in one function call we can do this instead:

let command = runAsync("command") { // do something when it finishes } command.stdout.onOutput = { stream in // "stream" is of type ReadableStream } command.stderror.onOutput = { stream in // "stream" is of type ReadableStream } This way the output handlers can be used on any ReadableStream, including main.stdin, which can be very useful.

BTW I'm not sure about the name "onOutput", what do you think? Especially main.stdin.onOutput looks weird, like we're getting output from the input. Which I guess we kind of are, but it looks self-contradictory.

Oh and I like the stop method, very useful.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214297494

[kareman]

Another thing: in

let command = runAsync("command") {
    // do something when it finishes
}

Is it obvious for anyone unfamiliar with SwiftShell that the contents of the closure will only be executed when the command finishes? We could add the completion handler as a property on AsyncShellTask so you would write this instead:

let command = runAsync("command")
    .onCompletion {
    // do something when it finishes
    }

[kareman]

This way we can add the completion handler after executing the command. And also cancel the completion handler later with command.onCompletion(nil) or command.onCompletion { _ in }.

[kenthinson]

Good idea Sensei :)

On Apr 25, 2016, at 6:00 AM, Kare Morstol notifications@github.com wrote:

Another thing: in

let command = runAsync("command") { // do something when it finishes } Is it obvious for anyone unfamiliar with SwiftShell that the contents of the closure will only be executed when the command finishes? We could add the completion handler as a property on AsyncShellTask so you would write this instead:

let command = runAsync("command") .onCompletion { // do something when it finishes } — You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214305115

[kenthinson]

So got a question / idea to run by you before I go a write it up. Considering we are going with command.onCompletion() should we go with command.onOutput() and command.onError()? The reason I’m thinking this is that if they are all next to each other it’s easy to access them and see what options you have with autocomplete.

However if you think it makes more sense to make them as methods of stdout and stderror I can do that instead. I will need to store a reference to the calling AsyncShelltask in ReadableStream to pass back to the API user. I’m guessing use a optional property? or should it be part of ReadableStream.init() with a default value?

Let me know what you think is best.

Sorry I snuck in the Int change there, it was just annoying me in my own app ;P The Semantic Versioning site you sent me is really good info. I’ll make sure to read the whole thing when I have more time. I’ve just skimmed it so far. I’m thinking it would only affect people who are using the int32 and not casting immediately like I am in my app. However messing with them without notice it not very nice :) Would that be something to mark as Deprecated some how? I see Apple do that in their API.

Handeler Doh! copy and paste got me again lol.

Thanks for the feedback Code Fu Sensei!

On Apr 25, 2016, at 5:38 AM, Kare Morstol notifications@github.com wrote:

Hi, this is excellent work and a really useful addition. Better handling of asynchronous commands is definitely something SwiftShell needs.

Some thoughts:

There are too many "e"s in "Handeler", it should be “Handler”. I completely agree that ShellError.errorcode should be an Int, it is already in my list of things that should be fixed in a future version. The problem is this is a backwards incompatible change, meaning current scripts and applications that use SwiftShell 2 might no longer compile if we make this change now. If we follow semantic versioning http://semver.org/ this change will have to wait for SwiftShell 3. This is a small change which will affect very few people, if any, so I'm quite possibly too cautious here but in any case this change would be better off in a separate pull request.

BTW, adding the completion handler to runAsync is backwards compatible because we are adding a new parameter with a default value so existing code will still compile. We just have to release it as SwiftShell 2.1.

I think the output handler and error handler should be moved to ReadableStream. That way, instead of having to provide 3 closures in one function call we can do this instead:

let command = runAsync("command") { // do something when it finishes } command.stdout.onOutput = { stream in // "stream" is of type ReadableStream } command.stderror.onOutput = { stream in // "stream" is of type ReadableStream } This way the output handlers can be used on any ReadableStream, including main.stdin, which can be very useful.

BTW I'm not sure about the name "onOutput", what do you think? Especially main.stdin.onOutput looks weird, like we're getting output from the input. Which I guess we kind of are, but it looks self-contradictory.

Oh and I like the stop method, very useful.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214297494

[kenthinson]

I had one more question if we do for example command.onOutput() or command.stdout.onOutput() instead of having it inside the init. is there a possibility of a race condition. In other words missing output because we are passing the handler after init()? I guess it would be a small chance as NSTask would have to return data very fast for that to happen?

Sent from my iPhone

On Apr 25, 2016, at 6:06 AM, Kare Morstol notifications@github.com wrote:

command.onCompletion(nil)

[kareman]

That's true, the functionality would be easier to find with autocompletion. But we also need to be able to use output handlers outside of runAsync, like in main.stdin and ShellContext. With the latter we can setup everything concerning a command before it is even run.

We could do both, implement onOutput on ReadableStream and have command.onError and command.onOutput as convenience methods. But I'm worried this

let command = runAsync("command")
command.onError { stream in
    ...
}

makes it seem like command.onError is called when there is an error, as in the command has exited with an error code. But commands can print something to stderror and still return without an error code (exitcode == 0).

ReadableStream doesn't need to know about AsyncShellTask if we do something like this:

extension AsyncShellTask {
    public func onCompletion ( handler: ((AsyncShellTask) -> ())? ) -> AsyncShellTask {
        task.terminationHandler = { [unowned self] _ in
            handler?(self)
        }
        return self
    }
}

extension ReadableStream {
    public func onOutput ( handler: ((ReadableStream) -> ())? ) {
        filehandle.readabilityHandler = { [unowned self] _ in
            handler?(self)
        }
    }
}

And use it like e.g. this:

let command = runAsync("ls")
    .onCompletion { c in
        print("finished!")
    }
command.stdout.onOutput { stream in
    print(stream)
}

---

You're absolutely right about ShellError.errorcode changing type to Int only affecting a few people (could even be no one as far as we know). And it is annoying to use the way it is now, both externally and internally. It may very well be that the total amount of annoyance produced will be less if we just change it now and release it in SwiftShell 2.1, so on 2nd thought I think you can just keep that change as is.

---

Good question about a race condition. I just did a quick test

let (writer,reader) = streams()
writer.writeln("one")
reader.onOutput { r in print("onOutput:", r.readSome()) }
writer.writeln("two")
// prints this:
// onOutput: Optional("one\ntwo\n")

and it seems like filehandle.readabilityHandler is not called right away if there already is output available when it is assigned to. But that output is not lost, it is still there and will be included the next time read or readSome are called. filehandle.readabilityHandler does not read from the file handle, it is just called whenever there is something available to read.

[kenthinson]

I see where you are going with the naming makes sense.

extension ReadableStream { public func onOutput ( handler: ((ReadableStream) -> ())? ) { filehandle.readabilityHandler = { [unowned self] _ in handler?(self) } } }

Hmmm. So example. I create an array of AsyncShellTask. They are all running the same shell executable but with different options a bunch of rsync, or ssh for example. I want to get the output from each instance of rsync and use it in a GUI interface. When the handler gets called I need to know what AsyncShellTask the output is coming from. not just the output data. that is why I was thinking I need a reference to the AsyncShellTask itself. I guess I could check each instance of ReadableStream on each AsyncShellTask in my array to see if it’s the same as the senders.

Also do you prefer I write my additions as extension? :)

On Apr 25, 2016, at 5:04 PM, Kare Morstol notifications@github.com wrote:

That's true, the functionality would be easier to find with autocompletion. But we also need to be able to use output handlers outside of runAsync, like in main.stdin and ShellContext. With the latter we can setup everything concerning a command before it is even run.

We could do both, implement onOutput on ReadableStream and have command.onError and command.onOutput as convenience methods. But I'm worried this

let command = runAsync("command") command.onError { stream in ... } makes it seem like command.onError is called when there is an error, as in the command has exited with an error code. But commands can print something to stderror and still return without an error code (exitcode == 0).

ReadableStream doesn't need to know about AsyncShellTask if we do something like this:

extension AsyncShellTask { public func onCompletion ( handler: ((AsyncShellTask) -> ())? ) -> AsyncShellTask { task.terminationHandler = { [unowned self] _ in handler?(self) } return self } }

extension ReadableStream { public func onOutput ( handler: ((ReadableStream) -> ())? ) { filehandle.readabilityHandler = { [unowned self] _ in handler?(self) } } } And use it like e.g. this:

let command = runAsync("ls") .onCompletion { c in print("finished!") } command.stdout.onOutput { stream in print(stream) } You're absolutely right about ShellError.errorcode changing type to Int only affecting a few people (could even be no one as far as we know). And it is annoying to use the way it is now, both externally and internally. It may very well be that the total amount of annoyance produced will be less if we just change it now and release it in SwiftShell 2.1, so on 2nd thought I think you can just keep that change as is.

Good question about a race condition. I just did a quick test

let (writer,reader) = streams() writer.writeln("one") reader.onOutput { r in print("onOutput:", r.readSome()) } writer.writeln("two") // prints this: // onOutput: Optional("one\ntwo\n") and it seems like filehandle.readabilityHandler is not called right away if there already is output available when it is assigned to. But that output is not lost, it is still there and will be included the next time read or readSome are called. filehandle.readabilityHandler does not read from the file handle, it is just called whenever there is something available to read.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214568032

[kenthinson]

Scratch that. I think I’m too tired from being up all nigh. I would probably have the AsyncShellTask as a member of my own class, and have an array of my own classes.

On Apr 25, 2016, at 5:04 PM, Kare Morstol notifications@github.com wrote:

That's true, the functionality would be easier to find with autocompletion. But we also need to be able to use output handlers outside of runAsync, like in main.stdin and ShellContext. With the latter we can setup everything concerning a command before it is even run.

We could do both, implement onOutput on ReadableStream and have command.onError and command.onOutput as convenience methods. But I'm worried this

let command = runAsync("command") command.onError { stream in ... } makes it seem like command.onError is called when there is an error, as in the command has exited with an error code. But commands can print something to stderror and still return without an error code (exitcode == 0).

ReadableStream doesn't need to know about AsyncShellTask if we do something like this:

extension AsyncShellTask { public func onCompletion ( handler: ((AsyncShellTask) -> ())? ) -> AsyncShellTask { task.terminationHandler = { [unowned self] _ in handler?(self) } return self } }

extension ReadableStream { public func onOutput ( handler: ((ReadableStream) -> ())? ) { filehandle.readabilityHandler = { [unowned self] _ in handler?(self) } } } And use it like e.g. this:

let command = runAsync("ls") .onCompletion { c in print("finished!") } command.stdout.onOutput { stream in print(stream) } You're absolutely right about ShellError.errorcode changing type to Int only affecting a few people (could even be no one as far as we know). And it is annoying to use the way it is now, both externally and internally. It may very well be that the total amount of annoyance produced will be less if we just change it now and release it in SwiftShell 2.1, so on 2nd thought I think you can just keep that change as is.

Good question about a race condition. I just did a quick test

let (writer,reader) = streams() writer.writeln("one") reader.onOutput { r in print("onOutput:", r.readSome()) } writer.writeln("two") // prints this: // onOutput: Optional("one\ntwo\n") and it seems like filehandle.readabilityHandler is not called right away if there already is output available when it is assigned to. But that output is not lost, it is still there and will be included the next time read or readSome are called. filehandle.readabilityHandler does not read from the file handle, it is just called whenever there is something available to read.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214568032

[kareman]

When you create the closure you just refer to the

[kareman]

Asyncshelltask in it and it will be captured. You don't need it as a parameter to the closure.

[kenthinson]

Haha it’s so simple I didn’t even think of it.

On Apr 25, 2016, at 6:25 PM, Kare Morstol notifications@github.com wrote:

Asyncshelltask in it and it will be captured. You don't need it as a parameter to the closure.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214579994

[kenthinson]

is this ok?

extension ReadableStream { public func onOutput ( handler: ((String) -> ())? ) { if let h = handler{ filehandle.readabilityHandler = {(NSFileHandle) in if let output = self.readSome(){ h(output) } } }

I figured if you are trying to get this callback you are after the string that is currently in the buffer? Again I might be overlooking something :/

On Apr 25, 2016, at 6:25 PM, Kare Morstol notifications@github.com wrote:

Asyncshelltask in it and it will be captured. You don't need it as a parameter to the closure.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/kareman/SwiftShell/pull/17#issuecomment-214579994

[kareman]

Oh and yes extensions are nice.

[kenthinson]

So I tried this code and I get a error that look like a memory issue.

On Apr 25, 2016, at 5:04 PM, Kare Morstol notifications@github.com wrote:

extension AsyncShellTask { public func onCompletion ( handler: ((AsyncShellTask) -> ())? ) -> AsyncShellTask { task.terminationHandler = { [unowned self] _ in handler?(self) } return self } }

let test = runAsync("ping","-c 5", "google.com").onCompletion({(task:AsyncShellTask) in print(task.exitcode())})

libsystem_kernel.dylib`mach_msg_trap: 0x7fff8793df68 <+0>: movq %rcx, %r10 0x7fff8793df6b <+3>: movl $0x100001f, %eax ; imm = 0x100001F 0x7fff8793df70 <+8>: syscall -> 0x7fff8793df72 <+10>: retq
0x7fff8793df73 <+11>: nop

I changed it to

extension AsyncShellTask { public func onCompletion ( handler: ((AsyncShellTask) -> ())? ) -> AsyncShellTask { task.terminationHandler = { (NSTask) in handler?(self) } return self } }

issue is resolved. Does this code look ok to you?

[kareman]

I put "[unowned self]" there to avoid a reference cycle, where the AsyncShellTask refers to the NSTask, which refers to the task.terminationHandler closure, which again refers back to the AsyncShellTask with self. But sometimes a reference cycle can be useful, like in this case where presumably the test variable went out of scope and was removed from memory before the command had finished.