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

Solve nil pointer derefrence #750

Merged
merged 3 commits into from
May 5, 2021
Merged

Solve nil pointer derefrence #750

merged 3 commits into from
May 5, 2021

Conversation

ersonp
Copy link
Contributor

@ersonp ersonp commented Apr 30, 2021

Did you run make format && make check?
yes

Fixes #740

Changes:

  • Initialised Summary struct inside ExtraSummary

How to test this PR:

  1. Setup hypervisor
  2. Connect visor to the hypervisor
  3. Disconnect visor
  4. Check hypervisor logs

@jdknives
Copy link
Member

@ersonp this solves the panic, but leaves us with another issue:

Sometimes, the visor that got shutdown is not shown as offline and the frontend cant reload anymore.

log.txt

@ersonp
Copy link
Contributor Author

ersonp commented Apr 30, 2021

I have managed to fix that issue by sending empty fields. But I think that's unnecessary data, and would be better if there is a fix from the UI side. Having said that, it might not make a lot of difference and we can keep using this fix.

@ersonp
Copy link
Contributor Author

ersonp commented May 3, 2021

@i-hate-nicknames can we use extraSummaryResp with IsHypervisor and DmsgStats fields instead of using a new object extraSummaryWithDmsgResp.
Also, I feel like few of the method names can be changed for better clarity like: getVisorsExtraSummary to getAllVisorsSummary and makeExtraSummaryResp tp generateSummary.

@jdknives
Copy link
Member

jdknives commented May 3, 2021

@ersonp @i-hate-nicknames just a general comment from my side:

While reviewing the fix from Erson, I thought that the naming for the various summaries could be clearer. Currently it looks to me like there are multiple summaries which leads us to weird constructs such as ExtraSummaryWithDmsgResponse etc. In this case I think it may be better to call the most expansive (informative and detailed) struct the Summary and call the current Summary object Overview or Details. Maybe there is a better option as well.

@i-hate-nicknames
Copy link
Contributor

Yes, feels like it definitely needs some refactoring. There are multiple levels of extra that seems to have been added over time. Visor has summary and extra summary, and hypervisor has his own extra summary, plus response wrappers around visor summaries:
hypervisor provides:

  1. visor.Summary (via getVisor()), wraps in summaryResp
  2. visor.ExtraSummary (via getVisorSummary()), wraps in extraSummaryResp
  3. its own extra summary (extra-extra now?) (via getVisorsExtraSummary()), extraSummaryWithDmsgResp
    There are some overlaps between those structures but not always, e.g. visor.ExtraSummary Dmsg is different from Dmsg in extraSummaryWithDmsgResp. So we cannot simply embed one into other.

I would propose:

  1. Remove Dmsg from visor.ExtraSummary. This field is not populated by visor api, so there is no reason to have it there.
  2. Investigate why do we need dmsg stats for all visors in getVisorSummary, since we are asking for stats of a single visor.
    In case we don't need that data, extraSummaryWithDmsgResp and extraSummaryResp become equivalent and the former
    can be removed
  3. Factor out TCPAddr and Online fields into its own struct, maybe smth like NetworkStats?
  4. Rename summary to overview like @jdknives suggested, and extra summary to just summary

@jdknives jdknives merged commit d3a7d7d into skycoin:develop May 5, 2021
@Senyoret1 Senyoret1 mentioned this pull request May 13, 2021
@ersonp ersonp deleted the bug/hypervisor-panic branch April 11, 2022 15:37
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.

None yet

3 participants