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.

Sign up for GitHub

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

Jump to bottom

TestPipelineCall fails #420

Closed
lthibault opened this issue Jan 10, 2023 · 1 comment · Fixed by #424
Closed

TestPipelineCall fails #420

lthibault opened this issue Jan 10, 2023 · 1 comment · Fixed by #424
Labels
bug

Comments

@lthibault
Copy link
Collaborator 

=== RUN   TestPipelineCall
Error:     server_test.go:272: GetNumber() #1 = 1; want 0
Error:     server_test.go:278: GetNumber() #2 = 0; want 1

This is new, isn't it? ☝️

Originally posted by @lthibault in #418 (comment)
@lthibault lthibault mentioned this issue Jan 10, 2023
Merged
@lthibault lthibault added the bug label Jan 10, 2023
@zenhack
Copy link
Contributor

zenhack commented Jan 10, 2023

Git bisect blames 7844043

zenhack added a commit to zenhack/go-capnp that referenced this issue Jan 13, 2023
@zenhack
Split answer.sendReturn into prepare & complete steps.
974dc52 
No functional change, but this is in preparation to address capnproto#420; we
will have to separate these across two separate calls in Returner.
zenhack added a commit to zenhack/go-capnp that referenced this issue Jan 13, 2023
@zenhack
Fix capnproto#420
3b58955 
(without re-introducing capnproto#394)

420 was a regression introduced by our fix for 394; if we Return()
before we fulfill(), this can result in out of order message delivery,
since incoming calls can be made directly on clients in the result
before all queued calls are drained.

Previously, if we fulfilled() before Return(), we would get a data race
as fillPayloadCapTable modified the message while pipelined calls read
it. Having split Return(error) into PrepareReturn(error) and Return(),
it is now safe to move just the latter after the call to fulfill().
@zenhack zenhack mentioned this issue Jan 13, 2023
Merged
zenhack added a commit that referenced this issue Jan 13, 2023
@zenhack
Merge pull request #424 from zenhack/420
5022e46 
Fix #420
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix #420 zenhack/go-capnp
2 participants
@zenhack @lthibault