Skip to content

MakeTypedEncoder breaks when using re.SetError #65

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

Closed
dignifiedquire opened this issue Feb 15, 2018 · 7 comments · Fixed by #128
Closed

MakeTypedEncoder breaks when using re.SetError #65

dignifiedquire opened this issue Feb 15, 2018 · 7 comments · Fixed by #128

Comments

@dignifiedquire
Copy link
Member

If I use MakeTypedEncoder, with a concrete type, say *Foo then the error case is not handled correctly anymore, as SetError calls re.Emit but not with type *Foo but rather *cmdkit.Error.

This results in a panic of unexpected type: *cmdkit.Error

@Stebalien
Copy link
Member

Are you using this to encode responses sent over the network? The CLI response emitter should be special-casing errors and shouldn't be passing them to the encoder.

@dignifiedquire
Copy link
Member Author

dignifiedquire commented Mar 5, 2018 via email

@Stebalien
Copy link
Member

Hm. I wonder why I've never seen this in go-ipfs. But yeah, I see the bug.

@Stebalien
Copy link
Member

Should be fixed with #112.

@Stebalien
Copy link
Member

So, this isn't fixed. We considered simply removing errors from the main stream but didn't because we consider this a part of our "API".

I really don't see a sane fix.

@Stebalien
Copy link
Member

@whyrusleeping this can be reproduced with a simple:

curl -F @data="asdf" 'http://localhost:5001/api/v0/object/patch/append-data/Qm?encoding=text'

(and watch how IPFS panics)

Stebalien added a commit that referenced this issue Oct 25, 2018
Stebalien added a commit that referenced this issue Oct 25, 2018
Only use custom encoders for values.

fixes #65

Also, remove SetEncoder. Always derive the correct encoder from the request.
@Stebalien
Copy link
Member

So, I implemented what I consider to be an "acceptable" fix: #128.

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

Successfully merging a pull request may close this issue.

2 participants