Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Did I write this code to correctly be tail call optimized? Am I forcing computations more than I need with !? Did I generally write this asynchronously correctly to allow sends and receives to occur in their own time? Is this using multiple threads needlessly? Other general critiques?

open System
open System.Net.Sockets

let asyncGetInput = async { return BitConverter.GetBytes(Console.Read()) }
let rec asyncSendInput (stream : NetworkStream) = async {
    let! input = asyncGetInput
    stream.WriteByte |> Array.map <| input |> ignore
    do! asyncSendInput stream
    }

let asyncGetResponse (stream : NetworkStream) = async { return Char.ConvertFromUtf32(stream.ReadByte()) }
let rec asyncPrintResponse (stream : NetworkStream) = async {
        let! response = asyncGetResponse stream
        Console.Write(response)
        do! asyncPrintResponse stream
        }

#light
[<EntryPoint>]
let main args =
    let client = new System.Net.Sockets.TcpClient()
    client.Connect(args.[0], Int32.Parse(args.[1]))
    printfn "Connected to %A %A..." args.[0] args.[1]
    let stream = client.GetStream()
    printfn "Got stream, starting two way asynchronous communication."
    Async.Parallel [asyncSendInput stream; asyncPrintResponse stream] |> Async.RunSynchronously |> ignore
    0
share|improve this question
add comment

2 Answers

up vote 2 down vote accepted

It seems you are using asyncGetInput and asyncGetReponse to introduce concurrency, although this is a noble goal it is kind of useless because you are only using those from other workflows which are already running in the thread pool anyway. In that case it would be ok to make a synchronous call from inside those workflows.

The second point is about the do! at the end of asyncPrintResponse, you should use return! in your recursive call otherwise you will get a memory leak. I'm not sure if we can call this a bug because it can probably be considered normal that the definition of Do in the workflow isn't built for that. I've personally been corrected for that same error here: http://stackoverflow.com/questions/6905251/f-async-difference-between-two-structure

[edit] There's a difference between handling concurrency and introducing concurrency. If we ignore your code for a moment, you have two distinct inherently sequential workflows that you can parallelize and asyncSendInput and asyncPrintResponse is already doing that by itself, as such asyncGetInput and asyncGetResponse are unnecessary and will only create more lightweight thread that will end up being scheduled/deschedule from the thread pool.

Here's a little cleanup:

open System
open System.Net.Sockets

let rec asyncSendInput (stream : NetworkStream) =
    async {
        let input = Console.Read() |> BitConverter.GetBytes
        input |> Array.iter stream.WriteByte
        return! asyncSendInput stream
    }

let rec asyncPrintResponse (stream : NetworkStream) =
    async {
        let response = stream.ReadByte() |> Char.ConvertFromUtf32
        Console.Write(response)
        return! asyncPrintResponse stream
    }

[<EntryPoint>]
let main args =
    let client = new System.Net.Sockets.TcpClient()
    client.Connect(args.[0], Int32.Parse(args.[1]))
    printfn "Connected to %A %A..." args.[0] args.[1]
    let stream = client.GetStream()
    printfn "Got stream, starting two way asynchronous communication."
    asyncSendInput stream |> Async.Start
    asyncPrintResponse stream |> Async.RunSynchronously
    0
share|improve this answer
    
I had written this question off quite a while ago as one no one would ever bother to figure out! Thanks! –  Jimmy Hoffa Nov 23 '12 at 21:55
    
That said, you understand my goal in handling concurrency, any tips on what I could change to get them to cooperatively work with eachother in the same thread as I was trying, or is that going to require a lot more plumbing to create proper continuations? –  Jimmy Hoffa Nov 23 '12 at 21:57
    
Added some more info, understand that I didn't try to answer anything related to Sockets/Client/Server as I'm not familiar enough with them to give any useful feedback. –  David Grenier Nov 23 '12 at 22:10
1  
@JimmyHoffa Jon Harrop sent us here via the intertweets. :) –  Robert Jeppesen Nov 23 '12 at 23:47
add comment

The biggest problem with your code is that you can't really parse a char from a single byte if it is UTF32. Do you know if it is UTF32?

I find the backward pipe hard to read, and it really isn't needed here at all. Instead of ignoring the result, you can use iter instead of map. This conveys intent better.

let rec asyncSendInput (stream : NetworkStream) = async {
    let! input = asyncGetInput
    Array.iter stream.WriteByte input
    do! asyncSendInput stream
    }

This is subjective and minor, but I would probably get rid of the recursion here too. I think this reads better:

let rec asyncSendInput (stream : NetworkStream) = async {
   while true do
      let! input = asyncGetInput
      Array.iter stream.WriteByte input
   }

let rec asyncPrintResponse (stream : NetworkStream) = async {
   while true do
      let! response = asyncGetResponse stream
      Console.Write(response)
}

You're blocking a thread in ReadByte(). This can easily be asynced. I'd also probably get rid of asyncGetResponse entirely.

let rec asyncPrintResponse (stream : NetworkStream) = async {
   while true do
      let! bytes = stream.AsyncRead(1)
      Char.ConvertFromUtf32 (int bytes.[0]) |> printf "%A"
}
share|improve this answer
    
It's utf8, that was the only nearby conversion method I could dig up off hand; I know that bit is totally wrong but this was a quick hack more for figuring out how to make the async stuff play nice. Also I didn't realize F# had loops.. I like your asyncPrintResponse, very readable! Thanks for the tips, I'll have to comb over this and give it a shot! –  Jimmy Hoffa Nov 24 '12 at 0:04
    
+1 for the async Read and while true. You can also remove the rec in that case. A recursive call is useful if you want to carry an immutable state –  David Grenier Nov 24 '12 at 4:54
    
@DavidGrenier I learned haskell before meddling in F# so statelessness just seems.. prettier to me. How does mutual recursion between a reader and a printer function sound stylistically for F#? Would that be non-idiomatic? On a side note.. f# || F# ? –  Jimmy Hoffa Nov 26 '12 at 15:57
2  
No it's perfectly fine, yet the while true do is a little more pragmatic that's all. I too have to force myself to be a little more down to earth sometimes. F# –  David Grenier Nov 26 '12 at 20:41
    
@DavidGrenier I have to do that too. :) –  Robert Jeppesen Nov 26 '12 at 22:52
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.