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

bug: building on wasm fails to index Result correctly #157

Closed
kegsay opened this issue Feb 28, 2020 · 2 comments
Closed

bug: building on wasm fails to index Result correctly #157

kegsay opened this issue Feb 28, 2020 · 2 comments

Comments

@kegsay
Copy link

kegsay commented Feb 28, 2020

I'm a user of sjson which allows deleting keys. sjson relies on gjson.Get(json, path string) Result to pull out the key to delete, as per this file. Unfortunately, deleting keys does not work when GOOS=js GOARCH=wasm and silently fails (fails to find the key) because when building for this platform it uses gjson_gae.go as per the build tag:

//+build appengine js

which has a no-op implementation for fillIndex which is critical for sjson. Copy/pasting the function implementation from gjson_ngae.go works correctly. Please can you update the build tags to remove js, as the ngae version works just fine on WASM. Thanks.

References:

gjson/gjson_gae.go

Lines 8 to 10 in 1ed2249

func fillIndex(json string, c *parseContext) {
// noop. Use zero for the Index value.
}

@kegsay
Copy link
Author

kegsay commented Feb 28, 2020

Test case:

package m_test

import (
	"testing"

	"github.com/tidwall/sjson"
)

func TestDeleteJSONWASM(t *testing.T) {
	inputJSON := []byte(`{
		"key1": "a",
		"deleteme": {
			"b": "c"
		}
	}`)

	outputJSON, err := sjson.DeleteBytes(inputJSON, "deleteme")
	if err != nil {
		t.Fatalf("Failed to DeleteBytes: %s", err)
	}
	if len(inputJSON) == len(outputJSON) {
		t.Fatalf("DeleteBytes failed to delete the key")
	}
}

Produces:

$ go test .                                                                       
ok  	example.com/m	0.204s

$ GOOS=js GOARCH=wasm go test -exec="$(go env GOROOT)/misc/wasm/go_js_wasm_exec" .
--- FAIL: TestDeleteJSONWASM (0.00s)
    main_test.go:22: DeleteBytes failed to delete the key
FAIL
FAIL	example.com/m	0.726s
FAIL

kegsay added a commit to matrix-org/dendrite that referenced this issue Feb 28, 2020
Previously, all events would come down redacted because the hash
checks would fail. They would fail because sjson.DeleteBytes didn't
remove keys not used for hashing. This didn't work because of a build
tag which included a file which no-oped the index returned.

See tidwall/gjson#157

When it's resolved, let's go back to mainline.
@tidwall
Copy link
Owner

tidwall commented Feb 28, 2020

I was unaware the the js tag is now used by wasm builds. The js tag was included because gopherjs uses it, and gopherjs does not support the unsafe keyword. Also, Google App Engine (appengine) use to not support unsafe either, but my understanding is that it now does.

Based on gopherjs/gopherjs#894, the gopherjs team may be "limiting scope and culling contributions". So I'm not sure about the future of that project. But I predict that wasm will only grow in usage.

I think the best choice is to merge gjson_ngae.go into gjson.go and delete gjson_gae.go and gjson_ngae.go.

I just pushed a new build v1.6.0 (f042915) that supports wasm.

@tidwall tidwall closed this as completed Feb 28, 2020
kegsay added a commit to matrix-org/dendrite that referenced this issue Mar 2, 2020
kegsay added a commit to matrix-org/dendrite that referenced this issue Mar 6, 2020
* Use a fork of pq which supports userCurrent on wasm

* Use sqlite3_js driver when running in JS

* Add cmd/dendritejs to pull in sqlite3_js driver for wasm only

* Update to latest go-sqlite-js version

* Replace prometheus with a stub. sigh

* Hard-code a config and don't use opentracing

* Latest go-sqlite3-js version

* Generate a key for now

* Listen for fetch traffic rather than HTTP

* Latest hacks for js

* libp2p support

* More libp2p

* Fork gjson to allow us to enforce auth checks as before

Previously, all events would come down redacted because the hash
checks would fail. They would fail because sjson.DeleteBytes didn't
remove keys not used for hashing. This didn't work because of a build
tag which included a file which no-oped the index returned.

See tidwall/gjson#157

When it's resolved, let's go back to mainline.

* Use gjson@1.6.0 as it fixes tidwall/gjson#157

* Use latest gomatrixserverlib for sig checks

* Fix a bug which could cause exclude_from_sync to not be set

Caused when sending events over federation.

* Use query variadic to make lookups actually work!

* Latest gomatrixserverlib

* Add notes on getting p2p up and running

Partly so I don't forget myself!

* refactor: Move p2p specific stuff to cmd/dendritejs

This is important or else the normal build of dendrite will fail
because the p2p libraries depend on syscall/js which doesn't work
on normal builds.

Also, clean up main.go to read a bit better.

* Update ho-http-js-libp2p to return errors from RoundTrip

* Add an LRU cache around the key DB

We actually need this for P2P because otherwise we can *segfault*
with things like: "runtime: unexpected return pc for runtime.handleEvent"
where the event is a `syscall/js` event, caused by spamming sql.js
caused by "Checking event signatures for 14 events of room state" which
hammers the key DB repeatedly in quick succession.

Using a cache fixes this, though the underlying cause is probably a bug
in the version of Go I'm on (1.13.7)

* breaking: Add Tracing.Enabled to toggle whether we do opentracing

Defaults to false, which is why this is a breaking change. We need
this flag because WASM builds cannot do opentracing.

* Start adding conditional builds for wasm to handle lib/pq

The general idea here is to have the wasm build have a `NewXXXDatabase`
that doesn't import any postgres package and hence we never import
`lib/pq`, which doesn't work under WASM (undefined `userCurrent`).

* Remove lib/pq for wasm for syncapi

* Add conditional building to remaining storage APIs

* Update build script to set env vars correctly for dendritejs

* sqlite bug fixes

* Docs

* Add a no-op main for dendritejs when not building under wasm

* Use the real prometheus, even for WASM

Instead, the dendrite-sw.js must mock out `process.pid` and
`fs.stat` - which must invoke the callback with an error (e.g `EINVAL`)
in order for it to work:

```
    global.process = {
        pid: 1,
    };
    global.fs.stat = function(path, cb) {
        cb({
            code: "EINVAL",
        });
    }
```

* Linting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants