Fixes https://github.com/tendermint/tmlibs/issues/55
MemDB previously mistakenly set the actual DB pointer to nil
although that side effect is not visible to the outside world
since p is an identifier within the scope of just that function
call. However, @melekes and I had a discussion in which we
came to the conclusion that Close for an in-memory DB should
instead be a noop and not cause any data loss. See the
discussion on https://github.com/tendermint/tmlibs/pull/56.
short: flushing the bufio buffer is not enough to ensure data
consistency.
long:
Saving an entry to the WAL calls writeLine to append data to the
autofile group backing the WAL, then calls group.Flush() to flush that
data to persistent storage. group.Flush() in turn proxies to
headBuf.flush(), flushing the active bufio.BufferedWriter. However,
BufferedWriter wraps a Writer, not another BufferedWriter, and the way
it flushes is by calling io.Writer.Write() to clear the BufferedWriter's
buffer. The io.Writer we're wrapping here is AutoFile, whose Write
method calls os.File.Write(), performing an unbuffered write to the
operating system, where, I assume, it sits in the OS buffers awaiting
sync. This means that Wal.Save does not, in fact, ensure the saved
operation is synced to disk before returning.
See https://github.com/go-kit/kit/issues/164 for discussion of why
kitlog returns an error.
```
Package log is designed to be used for more than simple application info/warning/error logging; it's suitable for log-structured data in an e.g. Lambda architecture, where each invocation is important. I agree with you that if we were doing only application logging the error would be more noise than signal. But the scope of the package is larger than that.
```
Since we are doing only application logging and we're not checking
errors, it is safe to get rid them.
Fix rename /root/.tendermint_test/consensus_replay_test/priv_validator.json.new /root/.tendermint_test/consensus_replay_test/priv_validator.json: no such file or directory
Currently IsDirEmpty returns true, err if it encounters
any error after trying to os.Open the directory.
I noticed this while studying the code and recalled a bug
from an earlier project in which doing the exact same thing
on code without permissions would trip out and falsely report
that the directory was empty.
Given demo.go in https://play.golang.org/p/vhTPU2RiCJ
* Demo:
```shell
$ mkdir -p sample-demo/1 && touch sample-demo/2
$ echo "1st round" && go run demo.go sample-demo
$ sudo chown root sample-demo && sudo chmod 0700 sample-demo
$ echo "2nd round" && go run demo.go sample-demo
```
That then prints out
```shell
1st round
original:: empty: false err: <nil>
updated:: empty: false err: <nil>
2nd round
original:: empty: true err: open data/: permission denied
updated:: empty: false err: open data/: permission denied
```
where in "2nd round", the original code falsely reports that
the directory is empty but that's a permission error.
I could write a code test for it, but that test requires me to change
users and switch to root as a Go user so no point in complicating our
tests, but otherwise it is a 1-to-1 translation between shell and Go.
Found common http utils that were being multiply duplicated across
many libraries and since am moving things in basecoin/unstable to
add for more functionality, it's better to put them in one
place.
Utilities and tests added:
- [X] FparseJSON
- [X] FparseAndValidateJSON
- [X] ParseRequestJSON
- [X] ParseAndValidateRequestJSON
- [X] WriteCode
- [X] WriteError
- [X] WriteSuccess
- [X] ErrorResponse
During review from @ethanfrey, made updates:
* Removed tt.want since it was a distraction/artifact that made
the reviewer think the tests weren't testing for both failed
and passed results.
* Added ErrorWithCode as WithCode is a common options pattern
in Go that could cause confusion:
ErrorWithCode(error, int) ErrorResponse
* Using json.NewDecoder(io.Reader) error instead of
ioutil.ReadAll(io.Reader) to slurp all the bytes.
* Added more test scenarios to achieve 100% coverage of http.go
We use WriteFileAtomic in two places:
```
p2p/addrbook.go
338: err = cmn.WriteFileAtomic(filePath, jsonBytes, 0644)
types/priv_validator.go
162: err = WriteFileAtomic(privVal.filePath, jsonBytes, 0600)
```
and we don't need .bak in any of the above. We save priv_validator every
10ms and addrbook every 2 min.
This fixes the problem with base-16 encoded values which may start with
digits: 015AB.... In such cases, the parser recognizes them as numbers
but fails to parse because of the follow-up characters (AB).
```
failed to parse tm.events.type=Tx AND hash=136E18F7E4C348B780CF873A0BF43922E5BAFA63:
parse error near digit (line 1 symbol 31 - line 1 symbol 32):
"6"
```
So, from now on we should quote any values. This seems to be the way
Postgresql has chosen.
> why we need it?
most of our subscribers will be RPC WS subscribers, so if there are too
many, nothing wrong with rejecting to subscribe.
however, consensus reactor must be the first to subscribe, since its
work depends on the pubsub package.
query parser
use parser compiler to generate query parser
I used https://github.com/pointlander/peg which has a nice API and seems
to be the most popular Golang compiler parser using PEG on Github.
More about PEG:
- https://en.wikipedia.org/wiki/Parsing_expression_grammar
- https://github.com/PhilippeSigaud/Pegged/wiki/PEG-Basics
- https://github.com/PhilippeSigaud/Pegged/wiki/Grammar-Examples
rename
implement query match function
match function
uncomment test lines
add more test cases for query#Matches
fix int case
rename events to pubsub
add comment about cache
assertReceive helper to not block on receive in tests
fix bug with multiple conditions
uncomment benchmark
first results:
```
Benchmark10Clients-2 1000 1305493 ns/op 3957519 B/op 355 allocs/op
Benchmark100Clients-2 100 12278304 ns/op 39571751 B/op 3505 allocs/op
Benchmark1000Clients-2 10 124120909 ns/op 395714004 B/op 35005 allocs/op
```
124ms to publish message to 1000 clients. A lot.
use AST from query.peg.go
separate pubsub and query packages by using Query interface in pubsub
wrote docs and refactor code
updates from Frey's review
refactor type assertion to use type switch
cleanup during shutdown
subscriber should create output channel, not the server
overflow strategies, server buffer capacity
context as the first argument for Publish
log error
introduce Option type
update NewServer comment
move helpers into pubsub_test
increase assertReceive timeout
add query.MustParse
add more false tests for parser
add more false tests for query.Matches
parse numbers as int64 / float64
try our best to convert from other types
add number to panic output
add more comments
save commit
introduce client argument as first argument to Subscribe
> Why we do not specify buffer size on the output channel in Subscribe?
The choice of buffer size of N here depends on knowing the number of
messages server will receive and the number of messages downstream
subscribers will consume. This is fragile: if we publish an additional
message, or if one of the downstream subscribers reads any fewer
messages, we will again have blocked goroutines.
save commit
remove reference counting
fix test
test client resubscribe
test UnsubscribeAll
client options
[pubsub/query] fuzzy testing
do not print msg as it creates data race!
Alexis Sellier
Emmanuel Odeke
Ethan Buchman
Zach Ramsay
Anton Kaliaev
Jae Kwon
Ethan Frey
rigel rozanski