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

fix: add recover fix panic on call to method #333

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhaolunxiang123
Copy link

add recover fix export method panic

Log: add recover fix export method panic

add recover fix  export method panic

Log: add recover fix export method panic
@zhaolunxiang123 zhaolunxiang123 changed the title fix: add recover fix export method panic fix: add recover fix panic on call to method Aug 29, 2022
@guelfey
Copy link
Member

guelfey commented Sep 5, 2022

I have multiple issues with this:

  1. recover is only really a band-aid to put on panics. Where does the panic you're facing coming from? If it's from the library's code, I would rather fix it itself - can you share the panic trace? If it's from the application code, then you should probably rather fix it there, or at least put the recover there.
  2. I would definitely not expose the recover return value to the D-Bus method caller - this could be a system-level daemon exposing information to an unprivileged user.

add server err reply client

Log: add server err reply client
@zhaolunxiang123
Copy link
Author

zhaolunxiang123 commented Sep 5, 2022

I can understand why not expose the recover return value to the D-Bus method caller, I'm adding a new server error return . it's from the application code, but export method too many,i can't fix it together . I refer to the GO standard library NET/HTTP in https://github.com/golang/go/blob/bd5595d7fa4eb3e234aabeac554f2ba8f2a95790/src/net/http/server.go#L1834

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 this pull request may close these issues.

2 participants