search

aichaos / rivescript-go

A RiveScript interpreter for Go. RiveScript is a scripting language for chatterbots.
https://www.rivescript.com/
MIT License
60 stars 16 forks source link

Library not thread safe #10

Closed raviteja-ms closed 7 years ago

raviteja-ms commented 8 years ago

@kirsle This library is not thread safe. It panics and raises fatal error in multi threaded environment. This is because you are not handling concurrent read and write of maps which is not thread safe in golang. Please see these posts for reference:

  1. https://www.reddit.com/r/golang/comments/4a8y8c/go_16_fatal_error_concurrent_map_read_and_map/
  2. http://stackoverflow.com/questions/36167200/how-safe-are-golang-maps-for-concurrent-read-write-operations
  3. http://stackoverflow.com/questions/11063473/map-with-concurrent-access

I'm also posting the error message for your reference.


fatal error: concurrent map read and map write

goroutine 51 [running]:
runtime.throw(0x89e4a0, 0x21)
    /home/ravi/Work/software/go/src/runtime/panic.go:530 +0x90 fp=0xc8206221f8 sp=0xc8206221e0
runtime.mapaccess1_faststr(0x727aa0, 0xc820011a10, 0xc82000a538, 0x2, 0x1)
    /home/ravi/Work/software/go/src/runtime/hashmap_fast.go:202 +0x5b fp=0xc820622258 sp=0xc8206221f8
github.com/aichaos/rivescript-go.(*RiveScript).processTags(0xc8200ce580, 0xc82000a538, 0x2, 0xc82019451a, 0xa, 0xc8200cb7a0, 0x16, 0xc8206228e0, 0x0, 0x0, ...)
    /home/ravi/Work/go/src/github.com/aichaos/rivescript-go/tags.go:227 +0xcd1 fp=0xc820622860 sp=0xc820622258
github.com/aichaos/rivescript-go.(*RiveScript).getReply(0xc8200ce580, 0xc82000a538, 0x2, 0xc82019451a, 0xa, 0x0, 0x1, 0x0, 0x0)
    /home/ravi/Work/go/src/github.com/aichaos/rivescript-go/brain.go:400 +0x28ce fp=0xc820622f10 sp=0xc820622860
github.com/aichaos/rivescript-go.(*RiveScript).processTags(0xc8200ce580, 0xc82000a538, 0x2, 0xc8204efbd8, 0x2, 0xc820194510, 0x30, 0xc82000a560, 0x1, 0x1, ...)
    /home/ravi/Work/go/src/github.com/aichaos/rivescript-go/tags.go:435 +0x22c7 fp=0xc820623518 sp=0xc820622f10
github.com/aichaos/rivescript-go.(*RiveScript).getReply(0xc8200ce580, 0xc82000a538, 0x2, 0xc8204efbd8, 0x2, 0x0, 0x0, 0x0, 0x0)
    /home/ravi/Work/go/src/github.com/aichaos/rivescript-go/brain.go:400 +0x28ce fp=0xc820623bc8 sp=0xc820623518
github.com/aichaos/rivescript-go.(*RiveScript).Reply(0xc8200ce580, 0xc82000a538, 0x2, 0xc8204efbd8, 0x2, 0x0, 0x0)
    /home/ravi/Work/go/src/github.com/aichaos/rivescript-go/brain.go:42 +0x47b fp=0xc820623d60 sp=0xc820623bc8

Please resolve this bug.

A suggestion from my side is to use concurrent-map instead of golang inbuilt maps.

kirsle commented 7 years ago

I'll prioritize this next time I work on this lib.

raviteja-ms commented 7 years ago

Ok... Please resolve it ASAP. Thanks..!!

raviteja-ms commented 7 years ago

@kirsle Any update..??

kirsle commented 7 years ago

Haven't had a chance to work on this module recently. I have an idea of how to resolve this issue, though, and that would also bring a useful new feature to the Go version: pluggable session managers.

In the Python version I added a pluggable session manager in PR #32. The main characteristics of this were:

This will centralize all reads and writes to user data into a small handful of functions in one place in the code. At that point it would be easy to use a mutex to ensure thread safety. And as a side effect it would bring the session manager feature from the Python version so that users can swap out the in-memory store with something backed by a database if they want.

It's mostly just a lot of busywork to apply the above fixes. I also did the work for the above for the JavaScript version (PR #137), but backed out on that because of JavaScript's heavy async nature not getting along well with how RiveScript manages user data.

In the mean time, consider using your own mutex at a level higher than where you call rs.Reply(). RiveScript is intended to reply very quickly to messages so hopefully locking at that level in your program shouldn't incur too much of a performance hit.

kirsle commented 7 years ago

I've opened PR #12 that implements the above and should fix your thread safety issue. Check it out and see if it resolves it, then I'll merge it.