Outgoing Request Hooks, swapping persistence layers#61
Conversation
rename hooks, add outgoing request hook, define chain types for node builder chooser tests
add persistence options to asyncloader & responsemanager + handling
Stebalien
left a comment
There was a problem hiding this comment.
basic post-merge review. LGTM but I'm missing the context I'd need to be sure.
| requestHook.hook(p, request, ha) | ||
| if ha.err != nil { | ||
| rm.requestHooksLk.RUnlock() | ||
| rm.persistenceOptionsLk.RUnlock() |
There was a problem hiding this comment.
uber-nit: I'd be consistent in the order in which we release these locks (not necessary at all, just slightly nicer).
| response := make(chan error, 1) | ||
| select { | ||
| case <-al.ctx.Done(): | ||
| return errors.New("context closed") |
There was a problem hiding this comment.
nit: "async loader closed". ditto below
| err := rpom.register(al) | ||
| select { | ||
| case <-al.ctx.Done(): | ||
| case rpom.response <- err: |
There was a problem hiding this comment.
isn't this channel guaranteed to be buffered?
| case al.incomingMessages <- &startRequestMessage{requestID}: | ||
| return errors.New("context closed") | ||
| case err := <-response: | ||
| return err |
There was a problem hiding this comment.
may be simpler to just guarantee that we process all inbound requests. incomingMessages isn't buffered anyways so we can guarantee that we send back a response for all incoming messages we receive.
| // fall back to local store | ||
| stream, loadErr := loader(link, ipld.LinkContext{}) | ||
| if stream != nil && loadErr == nil { | ||
| localData, loadErr := ioutil.ReadAll(stream) |
There was a problem hiding this comment.
This is safe as long as we're sure we trust the local store. Probably fine, just calling this out to make sure.
* build(scripts): add imports + release scripts * style(imports): fix import style use standard import style via script * ci(circle): add imports-check, cbor-gen-check * docs(CHANGELOG): add changelog with releases record
Goals
Background on the needs here:
Implementation
This builds on the previous addition of swapping the loader and node-builder-chooser on the response side with similar hooks on the request side. Because the request side has to deal with storing as well as loading, and in order to not end up with duplicate store operations, I've decided to uniquely identify loader/storer combos as alternate persistence options, distinguished by a unique string name. (toyed with trying to not require the string name, but as you know, go is not good with comparability in function types)
Implementation: