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

thread-safety and convenience of basePrint #7

Closed
ghost opened this issue Feb 24, 2015 · 6 comments
Closed

thread-safety and convenience of basePrint #7

ghost opened this issue Feb 24, 2015 · 6 comments

Comments

@ghost
Copy link

ghost commented Feb 24, 2015

The basePrint function in baselib.go currently writes its output directly to os.Stdout through fmt.Print and friends. This will cause problems when calling print from multiple goroutines/Lua threads. Notably, that multiple Print calls can have their output interwoven on the console when they both run in parallel. For example:

fmt.Println("foo and bar")
go fmt.Println("bar and foo")

Can, under certain conditions, give output like this:

foo abar and foo
nd bar

Additionally, it would be nice if the host application could specify a different output target, aside from the default os.Stdout. For this reason, I'm wondering if it is not more convenient and safer to have it use Go's log package instead. This package takes care of both problems at once.

The changes to this package can be as simple as:

func basePrint(L *LState) int {
    var buf bytes.Buffer

    top := L.GetTop()
    for i := 1; i <= top; i++ {
        fmt.Fprint(&buf, L.Get(i).String(), " ")
    }

    log.Println(buf.String())
    return 0
}

The host now has the guarantee that logging stuff is thread safe and the output can be redirected to any target by calling log.SetOutput(io.Writer).

@ghost
Copy link

ghost commented Feb 24, 2015

The above change does not keep the original Lua behaviour though, notably: the space after every item, no new line after the items, and printing to standard error rather than standard out by default.

Also, why does log have to be used? It would probably be better to give LState an io.Writer field and have it initialized to os.Stdout.

@ghost
Copy link
Author

ghost commented Feb 24, 2015

Yes, the change linked up there is for my own use.

The original function added a tab after each printed item instead of a space. And this does actually add a newline after the item set.

Giving LState its own io.Writer is certainly an option. Not ideal though. For instance, in my case, I want all output to written to a single log file. Which would mean each LState would get the same file descriptor. This doesn't solve the thread safety issue.

@andrewchambers
Copy link

@jteeuwen - That sounds like an issue with your own code, not gopher-lua. You could easily solve this with a mutex + your own print function.

The most suitable option is allowing a custom writer to be assigned per LState.

In your own code you can then add a function to lock and unlock the writer which is assigned.

@ghost
Copy link
Author

ghost commented Feb 25, 2015

I use the log package, which already deals with all of this. It was just a suggestion. It's up to yuin to decide what he deems reasonable.

@yuin
Copy link
Owner

yuin commented Feb 25, 2015

Threadings

First, the LState(including the print function) is not goroutine safe.I would recommend to use one LState per goroutine and communicate between goroutines by using channels.
And if not so, you have to implement a locking mechanism as andrewchambers remarks.

Default outputs

Lua reference manual says about the print function as follows:

Receives any number of arguments, and prints their values to stdout

If you want to print values to anywhere but stdout, use io.write and/or io.output.

Now, therefore, in consideration of the above, I would like to make no changes to GopherLua.

@andrewchambers
Copy link

I agree that you should keep with the standard.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants