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

Add typing notifications to /sync responses - fixes #635 #718

Merged
merged 25 commits into from Jul 12, 2019

Conversation

2 participants
@Cnly
Copy link
Collaborator

commented Jun 18, 2019

This PR adds a new consumer for typing notifications in syncapi. It also brings changes to syncserver.go and some related files so EDUs can better fit in /sync responses.

Fixes #635.
Fixes #574.

Signed-off-by: Alex Chen minecnly@gmail.com

Pull Request Checklist

  • I have added any new tests that need to pass to testfile as specified in docs/sytest.md
  • Pull request includes a sign off
Add typing notifications to /sync responses - fixes #635
Signed-off-by: Alex Chen <minecnly@gmail.com>

@Cnly Cnly force-pushed the Cnly:syncapi-typing-notifications-635 branch from f056bec to b896fdc Jun 25, 2019

@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Could you fix the linting? Cyclomatic complexity just means the function has too many branching statements (it is indeed annoying but helps keep functions to a reasonable number of lines).

Also during testing I received a stack trace whenever sending a typing event:

INFO[2019-06-25T13:31:27.257914853Z] Incoming request                              req.id=hsiy5pdFCqqy req.method=PUT req.path="/_matrix/client/r0/rooms/!aaLEoOz66BAshy1K:localhost/typing/@bla:localhost"
ERRO[2019-06-25T13:31:27.258729695Z] Request panicked!
goroutine 130 [running]:
runtime/debug.Stack(0xc000392cc0, 0xc0002192b8, 0xc000392d20)
	/usr/lib/go-1.11/src/runtime/debug/stack.go:24 +0xa7
github.com/matrix-org/util.Protect.func1.1(0xc0005c2700, 0xc05ec0, 0xc000652c40)
	/home/user/go/pkg/mod/github.com/matrix-org/util@v0.0.0-20171127121716-2e2df66af2f5/json.go:87 +0x16f
panic(0xa6a140, 0x1070190)
	/usr/lib/go-1.11/src/runtime/panic.go:513 +0x1b9
github.com/matrix-org/dendrite/typingserver/cache.(*TypingCache).GetTypingUsersIfUpdatedAfter(0xc000251b30, 0xc00001cab9, 0x1b, 0x0, 0x8, 0xc0002196c0, 0xc000219698, 0x8)
	/home/user/code/dendrite/typingserver/cache/cache.go:66 +0x8a
github.com/matrix-org/dendrite/typingserver/cache.(*TypingCache).GetTypingUsers(0xc000251b30, 0xc00001cab9, 0x1b, 0xc000032000, 0x203000, 0x203000)
	/home/user/code/dendrite/typingserver/cache/cache.go:53 +0x48
github.com/matrix-org/dendrite/typingserver/input.(*TypingServerInputAPI).sendEvent(0xc000251b60, 0xc000280a80, 0xe, 0xc00001cab9)
	/home/user/code/dendrite/typingserver/input/input.go:60 +0x5d
github.com/matrix-org/dendrite/typingserver/input.(*TypingServerInputAPI).InputTypingEvent(0xc000251b60, 0xc06b00, 0xc00044b6b0, 0xc000280a80, 0x109bae8, 0xa03020, 0xc00018c9f0)
	/home/user/code/dendrite/typingserver/input/input.go:56 +0xff
github.com/matrix-org/dendrite/clientapi/producers.(*TypingServerProducer).Send(0xc0002824a0, 0xc06b00, 0xc00044b6b0, 0xc00001cadc, 0xe, 0xc00001cab9, 0x1b, 0xc00001ca00, 0x0, 0xc00001cab9, ...)
	/home/user/code/dendrite/clientapi/producers/typingserver.go:49 +0x1bb
github.com/matrix-org/dendrite/clientapi/routing.SendTyping(0xc0005c2800, 0xc00044b770, 0xc00001cab9, 0x1b, 0xc00001cadc, 0xe, 0xc0001fe0f0, 0xc0002824a0, 0xb36e12, 0x6, ...)
	/home/user/code/dendrite/clientapi/routing/sendtyping.go:70 +0x2bb
github.com/matrix-org/dendrite/clientapi/routing.Setup.func17(0xc0005c2800, 0xc00044b770, 0xc0001fe0f0, 0xbff7e0, 0xc00019a960, 0x0)
	/home/user/code/dendrite/clientapi/routing/routing.go:187 +0x128
github.com/matrix-org/dendrite/common.MakeAuthAPI.func1(0xc0005c2800, 0x0, 0x0, 0x0, 0x0)
	/home/user/code/dendrite/common/httpapi.go:27 +0xb6
github.com/matrix-org/util.(*jsonRequestHandlerWrapper).OnIncomingRequest(0xc000196518, 0xc0005c2800, 0xc00012f680, 0x80, 0x80, 0xb05c20)
	/home/user/go/pkg/mod/github.com/matrix-org/util@v0.0.0-20171127121716-2e2df66af2f5/json.go:68 +0x33
github.com/matrix-org/util.MakeJSONAPI.func1(0xc05ec0, 0xc000652c40, 0xc0005c2800)
	/home/user/go/pkg/mod/github.com/matrix-org/util@v0.0.0-20171127121716-2e2df66af2f5/json.go:128 +0x89
github.com/matrix-org/util.Protect.func1(0xc05ec0, 0xc000652c40, 0xc0005c2700)
	/home/user/go/pkg/mod/github.com/matrix-org/util@v0.0.0-20171127121716-2e2df66af2f5/json.go:92 +0x8b
net/http.HandlerFunc.ServeHTTP(0xc000283350, 0xc05ec0, 0xc000652c40, 0xc0005c2700)
	/usr/lib/go-1.11/src/net/http/server.go:1964 +0x44
github.com/matrix-org/dendrite/common.MakeExternalAPI.func1(0xc05ec0, 0xc000652c40, 0xc0005c2600)
	/home/user/code/dendrite/common/httpapi.go:40 +0x190
net/http.HandlerFunc.ServeHTTP(0xc0002e6280, 0xc05ec0, 0xc000652c40, 0xc0005c2600)
	/usr/lib/go-1.11/src/net/http/server.go:1964 +0x44
github.com/gorilla/mux.(*Router).ServeHTTP(0xc00019a780, 0xc05ec0, 0xc000652c40, 0xc0005c2600)
	/home/user/go/pkg/mod/github.com/gorilla/mux@v1.3.0/mux.go:114 +0xe0
github.com/matrix-org/dendrite/common.WrapHandlerInCORS.func1(0xc05ec0, 0xc000652c40, 0xc0005c2400)
	/home/user/code/dendrite/common/httpapi.go:114 +0x169
net/http.HandlerFunc.ServeHTTP(0xc0004a8700, 0xc05ec0, 0xc000652c40, 0xc0005c2400)
	/usr/lib/go-1.11/src/net/http/server.go:1964 +0x44
net/http.(*ServeMux).ServeHTTP(0x107dc40, 0xc05ec0, 0xc000652c40, 0xc0005c2400)
	/usr/lib/go-1.11/src/net/http/server.go:2361 +0x127
net/http.serverHandler.ServeHTTP(0xc0004b60d0, 0xc05ec0, 0xc000652c40, 0xc0005c2400)
	/usr/lib/go-1.11/src/net/http/server.go:2741 +0xab
net/http.(*conn).serve(0xc0000a6140, 0xc06a40, 0xc0001922c0)
	/usr/lib/go-1.11/src/net/http/server.go:1847 +0x646
created by net/http.(*Server).Serve
	/usr/lib/go-1.11/src/net/http/server.go:2851 +0x2f5  context=missing panic="runtime error: invalid memory address or nil pointer dereference"
INFO[2019-06-25T13:31:27.258879396Z] Responding (35 bytes)                         code=500 context=missing

Cnly added some commits Jun 25, 2019

Fix linting
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix nil pointer and goroutine safety in typing cache
Signed-off-by: Alex Chen <minecnly@gmail.com>
Add warning for OnNewEvent when no user to wake up
Signed-off-by: Alex Chen <minecnly@gmail.com>
@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

Linting, complexity and nil pointer dereference should all be fixed now.

Cnly added some commits Jun 26, 2019

Fix syncapi/sync/notifier_test.go
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix more linting issues and docs
Signed-off-by: Alex Chen <minecnly@gmail.com>
Add a simple test for EDU-only updates in notifier_test.go
Signed-off-by: Alex Chen <minecnly@gmail.com>
Remove room ID from m.typing client events
Signed-off-by: Alex Chen <minecnly@gmail.com>
Better logging in typing event consumer
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix sync position with partial info used as complete one in syncserve…
…r and notifier

Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix necessary field not set properly in TypingEvent; clean up OutputT…
…ypingEvent

Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix latest sync pos used as since pos in requestpool.go
Previously, currPos, obtained using notifier.CurrentPosition(), is passed
to GetNotifyChannel. Since this is already the latest position on the
server, the client won't get updates until (1) a even newer event arrives at
the server, or (2) the /sync request times out.

(2) above is possible because when it times out, notifier calculates
currentSyncForUser based on syncReq, which contains the correct since pos
token that the client sent the server.

Signed-off-by: Alex Chen <minecnly@gmail.com>
Make Notifier.CurrentPosition() respect stream lock
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix docs for SyncPosition.WithUpdates
Signed-off-by: Alex Chen <minecnly@gmail.com>
Fix zero value of currPos may be used in OnIncomingSyncRequest
Signed-off-by: Alex Chen <minecnly@gmail.com>
Userstreams should store and use complete pos, not pos update ("pos d…
…elta")

Signed-off-by: Alex Chen <minecnly@gmail.com>
Remove users from typing list when typing status times out
Signed-off-by: Alex Chen <minecnly@gmail.com>

@anoadragon453 anoadragon453 requested a review from matrix-org/dendrite-core Jun 27, 2019

@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Jun 27, 2019

@anoadragon453 anoadragon453 moved this from In progress to Community PRs in Homeserver Task Board Jun 27, 2019

@anoadragon453 anoadragon453 self-assigned this Jun 27, 2019

Refine docs
Signed-off-by: Alex Chen <minecnly@gmail.com>
@Cnly

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 27, 2019

Just refined some docs. The PR is now ready for review! :)

Cnly added some commits Jul 1, 2019

Add newly passing tests to testfile
Signed-off-by: Alex Chen <minecnly@gmail.com>
Merge branch 'master' into syncapi-typing-notifications-635
Signed-off-by: Alex Chen <minecnly@gmail.com>
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/types/types.go Outdated
Show resolved Hide resolved syncapi/consumers/typingserver.go Outdated
Show resolved Hide resolved syncapi/consumers/typingserver.go
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/sync/request.go Outdated
Show resolved Hide resolved syncapi/types/types.go Outdated
Show resolved Hide resolved syncapi/types/types.go Outdated
Show resolved Hide resolved typingserver/input/input.go
@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

Also, please merge master into this branch.

(The fact testfile is so easy to conflict on makes me a little worried).

Cnly added some commits Jul 12, 2019

Docs and code cleanup
Signed-off-by: Alex Chen <minecnly@gmail.com>
Merge 'upstream/master' into syncapi-typing-notifications-635
Signed-off-by: Alex Chen <minecnly@gmail.com>

@Cnly Cnly requested a review from anoadragon453 Jul 12, 2019

@anoadragon453
Copy link
Member

left a comment

lgtm! other than just some small grammatical things

Show resolved Hide resolved syncapi/storage/syncserver.go Outdated
Show resolved Hide resolved syncapi/storage/syncserver.go Outdated
Show resolved Hide resolved syncapi/storage/syncserver.go Outdated
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved syncapi/sync/notifier.go Outdated
Show resolved Hide resolved typingserver/cache/cache.go Outdated

Cnly and others added some commits Jul 12, 2019

Apply suggestions from code review
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Apply suggestions from code review
Co-Authored-By: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>

@Cnly Cnly merged commit 29841be into matrix-org:master Jul 12, 2019

1 check passed

ci/circleci: dendrite Your tests passed on CircleCI!
Details

Homeserver Task Board automation moved this from Community PRs to Done Jul 12, 2019

Cnly added a commit to Cnly/dendrite that referenced this pull request Jul 19, 2019

Fix /sync may add EDUs for left rooms
In 29841be (matrix-org#718), EDUs are added to /sync responses for rooms listed
in joinedRoomIDs returned by addPDUDeltaToResponse. However this list
may contain rooms other than those currently joined.

Some variable renamings are done to make golangci-lint pass.

Signed-off-by: Alex Chen <minecnly@gmail.com>

@Cnly Cnly referenced this pull request Jul 19, 2019

Open

Fix /sync may add EDUs for left rooms #752

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.