fix: Set Context properly #582

kehiy · 2023-07-08T22:32:18Z

Description

This PR introduces a global context and passed it to all modules when the node starts. By shutting down the node, the context will be cancelled and all modules will receive the cancellation signal.

Related issue(s)

Fixes Set Context properly #368

amirvalhalla · 2023-07-09T21:35:32Z

@kehiy

I thinks there are 2 issues which you should fix in your code

in network/network.go when you call newNetwork you passed ctx,cancel which it's correct but after you created the network struct you used n.ctx in newDHTService(n.ctx, n.host, kadProtocolID, conf.Bootstrap, n.logger) and for 2 other sub methods just used ctx that all of them have same memory address for ctx but different style code. please use n.ctx for all of them to have a same style.
in sync/sync.go you used ctx:=context.Background() which I think it would be better to create a context with cancellation, therefore when you stop sync package it will call cancel() and Done() channel of ctx will raise.

@b00f what is your opinion?

network/network.go

b00f · 2023-07-09T22:02:29Z

node/node.go

@@ -46,7 +47,9 @@ func NewNode(genDoc *genesis.Genesis, conf *config.Config,
 		"version", version.Version(),
 		"network", genDoc.ChainType())

-	network, err := network.NewNetwork(conf.Network)
+	ctx, cancel := context.WithCancel(context.Background())


I think we need to add cancel to the node structure and call it when the node is shutting down

yes, I can do it, but just tell me what should I do about the network module cancel?

I did that but there is an issue, all stuff is fine but I got this warning:

the cancel function is not used on all paths (possible context leak)

I already called cancel in the stop node method.

I did that but there is an issue, all stuff is fine but I got this warning:

the cancel function is not used on all paths (possible context leak)

I already called cancel in the stop node method.

Has the Stop method been used anywhere?

I don't think so.
where should I use it?

in GUI: CTA Exit
in CLI: on interrupt signal

ok
I will add it

kehiy · 2023-07-09T22:05:07Z

@kehiy

I thinks there are 2 issues which you should fix in your code

in network/network.go when you call newNetwork you passed ctx,cancel which it's correct but after you created the network struct you used n.ctx in newDHTService(n.ctx, n.host, kadProtocolID, conf.Bootstrap, n.logger) and for 2 other sub methods just used ctx that all of them have same memory address for ctx but different style code. please use n.ctx for all of them to have a same style.

in sync/sync.go you used ctx:=context.Background() which I think it would be better to create a context with cancellation, therefore when you stop sync package it will call cancel() and Done() channel of ctx will raise.

@b00f what is your opinion?

sure, I think its true
probably i will fix it

b00f · 2023-07-09T22:05:15Z

@Ja7ad please review this PR

kehiy · 2023-07-09T22:59:13Z

@b00f I added some changes!
I think that what you want, you can review it now

codecov · 2023-07-09T23:01:54Z

Codecov Report

Merging #582 (d1c6ae9) into main (6a16d7d) will decrease coverage by 0.05%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##             main     #582      +/-   ##
==========================================
- Coverage   83.53%   83.49%   -0.05%     
==========================================
  Files         154      154              
  Lines        7252     7257       +5     
==========================================
+ Hits         6058     6059       +1     
- Misses        915      918       +3     
- Partials      279      280       +1

Ja7ad · 2023-07-10T05:27:10Z

network/network_test.go

@@ -104,7 +106,8 @@ func readData(t *testing.T, r io.ReadCloser, len int) []byte {
 }

 func TestStoppingNetwork(t *testing.T) {
-	net, err := NewNetwork(testConfig())
+	ctx := context.Background()


for test you can use context.ToDo

is that the just thing I should change?

is that the just thing I should change?

No, in other test change Background to ToDo.

ok, I will change it too
should I remove defer func for cancel() in node.go?

ok, I will change it too should I remove defer func for cancel() in node.go?

Yes

Mistakenly approved

kehiy · 2023-07-10T18:15:01Z

@b00f should we close this and follow the steps I provided and make a new pr?
or we can resolve them here?

b00f · 2023-07-10T18:56:04Z

Better to have a new PR. Thanks

kehiy · 2023-07-10T18:58:14Z

Better to have a new PR. Thanks

okay, after the script, I will send a new PR for it.

OmidHekayati · 2023-07-12T07:49:11Z

Personally, I don't like using Context to solve when the requirements are just about object life cycles!

As discuss here suggest having three methods in all struct as an object to solve the object life cycle. When we have the Deinit method, anyone can easily know how an object de-initializes and reduce magic codes!

For a more deep discussion, I think it is better to have a meeting if you agree with the overall idea.

b00f · 2023-07-13T01:58:20Z

For a more deep discussion, I think it is better to have a meeting if you agree with the overall idea.

We have some DEV-meeting from time to time with the development team. There's no specific time for these meetings, but we can discuss it at the next DEV-meeting.

Thanks for bringing it up @OmidHekayati

kehiy added 3 commits July 9, 2023 06:15

fix: global context

a0e98c9

fix: global context set in mods

2588871

feat: Set Context properly

583a1fc

kehiy requested a review from b00f July 8, 2023 22:32

fix: linter issue

7147b51

b00f previously approved these changes Jul 9, 2023

View reviewed changes

fix: remove cancel form network and add it to node

d1c6ae9

kehiy requested a review from b00f July 9, 2023 22:59

pactus-project deleted a comment from codecov bot Jul 9, 2023

Ja7ad reviewed Jul 10, 2023

View reviewed changes

b00f requested a review from Ja7ad July 10, 2023 14:37

b00f changed the title ~~feat: Set Context properly~~ fix: Set Context properly Jul 10, 2023

b00f closed this Jul 10, 2023

kehiy deleted the context branch July 10, 2023 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Set Context properly #582

fix: Set Context properly #582

kehiy commented Jul 8, 2023 •

edited by b00f

Loading

amirvalhalla commented Jul 9, 2023 •

edited

Loading

b00f Jul 9, 2023

kehiy Jul 9, 2023

kehiy Jul 9, 2023

Ja7ad Jul 10, 2023

kehiy Jul 10, 2023

Ja7ad Jul 10, 2023

kehiy Jul 10, 2023

kehiy commented Jul 9, 2023

b00f commented Jul 9, 2023

kehiy commented Jul 9, 2023

codecov bot commented Jul 9, 2023

Ja7ad Jul 10, 2023

kehiy Jul 10, 2023

Ja7ad Jul 10, 2023 •

edited

Loading

kehiy Jul 10, 2023

Ja7ad Jul 10, 2023

kehiy commented Jul 10, 2023

b00f commented Jul 10, 2023

kehiy commented Jul 10, 2023

OmidHekayati commented Jul 12, 2023

b00f commented Jul 13, 2023

fix: Set Context properly #582

fix: Set Context properly #582

Conversation

kehiy commented Jul 8, 2023 • edited by b00f Loading

Description

Related issue(s)

amirvalhalla commented Jul 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kehiy commented Jul 9, 2023

b00f commented Jul 9, 2023

kehiy commented Jul 9, 2023

codecov bot commented Jul 9, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ja7ad Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kehiy commented Jul 10, 2023

b00f commented Jul 10, 2023

kehiy commented Jul 10, 2023

OmidHekayati commented Jul 12, 2023

b00f commented Jul 13, 2023

kehiy commented Jul 8, 2023 •

edited by b00f

Loading

amirvalhalla commented Jul 9, 2023 •

edited

Loading

Ja7ad Jul 10, 2023 •

edited

Loading