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

[dyncall] added dyncall module #93

Merged
merged 1 commit into from
Aug 23, 2018
Merged

Conversation

florianlenz
Copy link
Contributor

Implemented the call module as described in #86

Verify

  • check if all tasks from the issue are done
  • test should pass

closes #86

@codecov-io
Copy link

codecov-io commented Aug 22, 2018

Codecov Report

Merging #93 into develop will increase coverage by 0.02%.
The diff coverage is 89.18%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #93      +/-   ##
===========================================
+ Coverage    56.83%   56.85%   +0.02%     
===========================================
  Files           69       70       +1     
  Lines         4147     4184      +37     
===========================================
+ Hits          2357     2379      +22     
- Misses        1295     1308      +13     
- Partials       495      497       +2
Impacted Files Coverage Δ
dyncall/dyncall_registry.go 89.18% <89.18%> (ø)
dapp/request_limitation/throttling.go 40% <0%> (-18%) ⬇️
backend/ws_transport.go 65.81% <0%> (-1.71%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91502b3...c076ffc. Read the comment docs.

case m := <-r.addModuleChan:
modules[m.CallID()] = m
case getCallMod := <-r.getModuleChan:
mod := modules[getCallMod.id]
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably check if map key exists first to avoid any potential panics

The current implementation might still work correctly in practice though, even without any changes

In the case of the key not existing in the modules map, nil will be sent to getCallMod.respChan so as long as it's properly handled, it shouldn't cause major issues

A proposed alternative that feels more safe in terms of potential panics

if mod, exists := modules[getCallMod.id]; exists {
    getCallMod.respChan <- mod
} else {
    getCallMod.respChan <- nil
}

Even though it might not make much of a difference in this use case, if CallModule was a struct with 2 fields, of type int and string, you wouldn't get a nil if the key is missing in the map with the unsafe usage, and you would get {0,"") as this would be corresponding to the default values of int and string type.

It's just that this time the default value of CallModule is nil because its an interface

@florianlenz florianlenz merged commit 4dfb880 into develop Aug 23, 2018
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.

[pangea] dynamic call module
3 participants