fix: create local doc cache loader #2620
Conversation
baha-ai
requested review from
llorllale,
fqutishat,
troyronda,
aholovko and
rolsonquadras
baha-ai
added
bug
Codecov Report
@@ Coverage Diff @@
## main #2620 +/- ##
==========================================
- Coverage 88.41% 88.39% -0.02%
==========================================
Files 264 265 +1
Lines 20614 20636 +22
==========================================
+ Hits 18225 18242 +17
- Misses 1400 1405 +5
Partials 989 989
Continue to review full report at Codecov.
|
| @@ -685,6 +711,25 @@ const jsonVCWithProperContexts = `{ | |||
| ] | |||
| }` | |||
|
|
|||
| // nolint | |||
| const jsonVCWithProperContexts2 = `{ | |||
There was a problem hiding this comment.
example picked from @llorllale's comment here
| // { | ||
| // name: "canonizing sample document with extra io.Reader cached dummy context", | ||
| // doc: jsonLDMultipleInvalidRDFs, | ||
| // result: canonizedSampleVP_extraContext, | ||
| // opts: []ProcessorOpts{ | ||
| // WithRemoveAllInvalidRDF(), WithExternalContext("http://localhost:8652/dummy.jsonld"), | ||
| // WithDocumentLoaderCache(map[string]interface{}{ | ||
| // "http://localhost:8652/dummy.jsonld": strings.NewReader(extraJSONLDContext), | ||
| // }), | ||
| // }, | ||
| // }, |
There was a problem hiding this comment.
Because the buffer in strings.NewReader() is not thread safe.
One would need to use a thread safe Reader implementation for this to work. Example: https://gist.github.com/jmackie/11570bdcd8a4c10d72619a5e1f21c5f8
I can probably add a note in the DocumentLoader constructor about this. But this is an edge case with seldom use. Users of the DocumentLoader need to be aware to avoid using strings.NewReader() as a sync.Map value.
There was a problem hiding this comment.
added a test SyncReader implementation and uncommented this case, the benchmark is always passing now.
baha-ai
force-pushed
the
1833
branch
3 times, most recently
from
25f6287 to
ac82bda
Compare
This change creates a local JSON-LD Document Cache loader using a cache of type *sync.Map to avoid 'error on cache writes' panics. This change also introduces the 'benchmark' Make target to support benchmarking code in the framework. closes hyperledger-archives#1833 for the fatal error mentioned in the comment Also related and closes the discussion about the same panic topic in hyperledger-archives#2487 Signed-off-by: Baha Shaaban <[email protected]>
|
|
||
| cdl.rwMutex.RUnlock() | ||
|
|
||
| docFromLoader, err := cdl.nextLoader.LoadDocument(u) |
There was a problem hiding this comment.
Looks like nextLoader must also be thread-safe.
This will be an issue when people want to add extra contexts in downstream projects and they understandably forget about this bug, such as when they use the DocumentVerifier directly (like zcaps do).
The better solution is to have the user prepare the contexts themselves in an ld.DocumentLoader and be done with this. No need for locking or any of this extra complexity.
This change creates a local JSON-LD Document Cache loader using a cache of type *sync.Map
to avoid 'error on cache writes' panics.
This change also introduces the 'benchmark' Make target to support benchmarking code in the framework.
closes #1833 for the fatal error mentioned in the comment
Also related and closes the discussion about the same panic topic in #2487
Signed-off-by: Baha Shaaban [email protected]