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

fix: state listener observe writes at wrong time #13516

Merged
merged 30 commits into from
Dec 2, 2022
Merged

fix: state listener observe writes at wrong time #13516

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ac86866
fix: state listener observe writes at wrong time
yihuang Oct 12, 2022
cfc62ff
synchronous abci call, and format doc
yihuang Nov 27, 2022
faa1f62
fix comment
yihuang Nov 27, 2022
473a066
update file streamer readme and fix typos
yihuang Nov 27, 2022
3e78836
typo
yihuang Nov 27, 2022
94591f3
fix: state listener observe writes at wrong time
yihuang Oct 12, 2022
9d4b776
improve UX of file streamer, make it immediately usable after enabled
yihuang Nov 28, 2022
1ee67d8
Merge remote-tracking branch 'fork/fix-listener-main3' into fix-liste…
yihuang Nov 28, 2022
25ce3af
get homePage from opts
yihuang Nov 28, 2022
2ca7afc
Merge branch 'main' into fix-listener-main3
tac0turtle Nov 28, 2022
135aaaf
Merge branch 'main' into fix-listener-main3
yihuang Nov 29, 2022
eff8761
Merge remote-tracking branch 'origin/main' into fix-listener-main3
yihuang Nov 29, 2022
b168d6f
fix merge
yihuang Nov 29, 2022
d310a03
use fmt.Errorf
yihuang Nov 29, 2022
c752120
Update CHANGELOG.md
yihuang Nov 29, 2022
4a2f933
Update CHANGELOG.md
yihuang Nov 29, 2022
43ae889
Merge branch 'main' into fix-listener-main3
yihuang Nov 29, 2022
ce17fe6
Merge branch 'main' into fix-listener-main3
yihuang Nov 30, 2022
b7e5c4a
Merge branch 'main' into fix-listener-main3
tac0turtle Nov 30, 2022
33d5ca9
Merge branch 'main' into fix-listener-main3
tac0turtle Nov 30, 2022
f583c69
Merge branch 'main' into fix-listener-main3
yihuang Nov 30, 2022
932680b
fix unit test
yihuang Nov 30, 2022
4fa2fbb
type assertion -> type conversion
yihuang Dec 1, 2022
e312225
update changelog about error propogation
yihuang Dec 1, 2022
23e3f9b
Update CHANGELOG.md
yihuang Dec 1, 2022
c5dbfd5
Merge branch 'main' into fix-listener-main3
yihuang Dec 1, 2022
d14c4ae
Merge branch 'main' into fix-listener-main3
tac0turtle Dec 2, 2022
c5d5408
Merge branch 'main' into fix-listener-main3
tac0turtle Dec 2, 2022
20aa3f7
Merge branch 'main' into fix-listener-main3
tac0turtle Dec 2, 2022
3559e7d
Merge branch 'main' into fix-listener-main3
tac0turtle Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
synchronous abci call, and format doc
  • Loading branch information
@yihuang
yihuang committed Nov 27, 2022
commit cfc62ffce2c7e4d49c991edde777abc0b940cd44
8 changes: 4 additions & 4 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg
// call the hooks with the BeginBlock messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenBeginBlock(app.deliverState.ctx, req, res); err != nil {
app.logger.Error("BeginBlock listening hook failed", "height", req.Header.Height, "err", err)
panic(sdkerrors.Wrapf(err, "BeginBlock listening hook failed, height: %d", req.Header.Height))
github-advanced-security[bot] marked this conversation as resolved.
Fixed Show resolved Hide resolved
}
}

Expand All @@ -227,7 +227,7 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
// call the streaming service hooks with the EndBlock messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenEndBlock(app.deliverState.ctx, req, res); err != nil {
app.logger.Error("EndBlock listening hook failed", "height", req.Height, "err", err)
panic(sdkerrors.Wrapf(err, "EndBlock listening hook failed, height: %d", req.Height))
Fixed Show fixed Hide fixed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
}

Expand Down Expand Up @@ -330,7 +330,7 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliv
defer func() {
for _, streamingListener := range app.abciListeners {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't your fault @yihuang but app.abciListeners is both read-from and written-to without synchronization, which is a memory model violation. Reads occur throughout baseapp/abci.go and writes occur at baseapp/options.go SetStreamingService.

Copy link
Collaborator Author

@yihuang yihuang Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the abci methods are not called concurrently, the SetStreamingService is supposed to be called on initialization only. So I believe we don't need to be thread-safe here.

Basically the abciListeners is supposed to be initialized on startup, then read-only after.

if err := streamingListener.ListenDeliverTx(app.deliverState.ctx, req, res); err != nil {
app.logger.Error("DeliverTx listening hook failed", "err", err)
panic(sdkerrors.Wrap(err, "DeliverTx listening hook failed"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
}
}()
Expand Down Expand Up @@ -382,7 +382,7 @@ func (app *BaseApp) Commit() abci.ResponseCommit {
// call the hooks with the Commit message
for _, streamingListener := range app.abciListeners {
alexanderbez marked this conversation as resolved.
Show resolved Hide resolved
if err := streamingListener.ListenCommit(app.deliverState.ctx, res); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, ListenCommit should take a native Go context.Context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

app.logger.Error("Commit listening hook failed", "height", header.Height, "err", err)
panic(sdkerrors.Wrapf(err, "Commit listening hook failed, height: %d", header.Height))
}
}

Expand Down
26 changes: 13 additions & 13 deletions docs/architecture/adr-038-state-listening.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,14 @@ func (app *BaseApp) BeginBlock(req abci.RequestBeginBlock) (res abci.ResponseBeg

...

defer func() {
// call the hooks with the BeginBlock messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenBeginBlock(app.deliverState.ctx, req, res); err != nil {
panic(sdkerrors.Wrapf(err, "BeginBlock listening hook failed, height: %d", req.Header.Height))
}
}
}()
defer func() {
// call the hooks with the BeginBlock messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenBeginBlock(app.deliverState.ctx, req, res); err != nil {
panic(sdkerrors.Wrapf(err, "BeginBlock listening hook failed, height: %d", req.Header.Height))
}
}
}()

return res
}
Expand All @@ -311,7 +311,7 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
// Call the streaming service hooks with the EndBlock messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenEndBlock(app.deliverState.ctx, req, res); err != nil {
panic(sdkerrors.Wrapf(err, "EndBlock listening hook failed, height: %d", req.Height))
panic(sdkerrors.Wrapf(err, "EndBlock listening hook failed, height: %d", req.Height))
}
}
}()
Expand All @@ -326,9 +326,9 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliv
defer func() {
// call the hooks with the DeliverTx messages
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenDeliverTx(app.deliverState.ctx, req, res); err != nil {
panic(sdkerrors.Wrap(err, "DeliverTx listening hook failed"))
}
if err := streamingListener.ListenDeliverTx(app.deliverState.ctx, req, res); err != nil {
panic(sdkerrors.Wrap(err, "DeliverTx listening hook failed"))
}
}
}()

Expand Down Expand Up @@ -357,7 +357,7 @@ func (app *BaseApp) Commit() abci.ResponseCommit {
// call the hooks with the Commit message
for _, streamingListener := range app.abciListeners {
if err := streamingListener.ListenCommit(app.deliverState.ctx, res); err != nil {
panic(sdkerrors.Wrapf(err, "Commit listening hook failed, height: %d", header.Height)
panic(sdkerrors.Wrapf(err, "Commit listening hook failed, height: %d", header.Height))
}
}

Expand Down