Skip to content

stakepoold: Add shutdown context#528

Merged
dajohi merged 1 commit intodecred:masterfrom
JoeGruffins:stakepcontext
Sep 25, 2019
Merged

stakepoold: Add shutdown context#528
dajohi merged 1 commit intodecred:masterfrom
JoeGruffins:stakepcontext

Conversation

@JoeGruffins
Copy link
Copy Markdown
Member

A struct channel was used in order to signal shutdowns. This changes
that to a context.Context that we can spawn new contexts from. Abstracts
the task to a seperate file signal.go

This is pretty much a copy/paste from dcrd and dcrwallet. This gives us a base context to spawn new contexts off of.

One difference is that the name "ctx" is taken. Can we rename all "ctx" to something else?

Also, no idea what to do with the copyrights. Help me out.

@JoeGruffins JoeGruffins force-pushed the stakepcontext branch 2 times, most recently from 74eeda4 to 0c07f13 Compare September 14, 2019 23:03
Comment thread backend/stakepoold/signal.go Outdated
Comment thread backend/stakepoold/signalsigterm.go Outdated
@dajohi dajohi added this to the v1.3.0 milestone Sep 18, 2019
Comment thread backend/stakepoold/server.go Outdated
go ctx.WinningTicketHandler()
go ctx.NewTicketHandler(shutdownContext.Done())
go ctx.SpentmissedTicketHandler(shutdownContext.Done())
go ctx.WinningTicketHandler(shutdownContext.Done())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be possible for each of these methods to create their own shutdownContext using withShutdownCancel, otherwise this isn't much different from what we currently have.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It is the same. This is about readability and standardization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see. We could go the extra mile and have those 3 methods and any other method that requires a context, create their own shutdownContext using withShutdownCancel. What do you think?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

hmmm, idk. Would that be better? I'll look a little more into when the other decred repo's might spawn a new context. When I say standardization, I mostly mean what wallet and dcrd are doing. I think there a lot of good ideas that have developed that we could make use of. Although this particular refactor has been around since btcd apparently, just without the context. It was just a channel at first.
decred/dcrwallet@b145868#diff-dca9e25a1ecb5a565b1bf9142c755f71

Comment thread backend/stakepoold/server.go Outdated

// Wait for CTRL+C to signal goroutines to terminate via quit channel.
// Wait for CTRL+C to signal goroutines to terminate
<-shutdownContext.Done()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If my previous comment is applied, we'd only need to ctx.Wg.Wait() here without waiting for the shutdownContext to be canceled.

Actually, waiting for this shutdownContext to be canceled isn't relevant either ways, since we'll still have to wait for the waitgroup counter to return to 0; and each of the goroutines that decrement the waitgroup counter do so only after the shutdownContext is canceled.

If there's something we should do here, is make the waitgroup counter increment whenever a shutdown context is created, then ensure that each method that creates and uses a shutdown context decrements the waitgroup after performing any necessary cleanups on receiving the context cancelation signal. Then here, we only wait for the waitgroup counter to decrement to 0.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

With the latest changes which have been pushed, I think the wait for shutdownContext is now redundant and can be removed. There is no harm in keeping it there, but it is not necessary. Do you concur @itswisdomagain @JoeGruffins ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like it, but it is indeed redundant. Removing...

Copy link
Copy Markdown
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Tested the change and this works well.

Rough rule of thumb for copyrights is to not remove or modify anything which already exists, only add "Decred Developers" and/or the current year if either are missing. Strictly speaking, this is not exactly the right thing to do because "Decred Developers" is not a well defined group and not a legal entity, however we will stick to this pattern simply because it is present in the rest of the project. Consistency is king.

Agree that we should call our Context object ctx and I see you are starting to address that in #529.

I added a comment to update the copyright, but after that is fixed I'm happy for this PR to be merged as-is. This is definitely a step forward, and consistency with other repositories is always nice. The suggestion from @itswisdomagain is interesting, I guess this can be explored in another PR, perhaps after #529.

Comment thread backend/stakepoold/signal.go
@JoeGruffins
Copy link
Copy Markdown
Member Author

JoeGruffins commented Sep 23, 2019

So, I went a little farther. Let me know what you think, I can always pop off the last commit.
Move waitgroup to the server. This is done because It's the server waiting, and I think it works well to put the wg there.
Pass the waitgroup around by reference. This isn't something I've seen done, but it makes sense to me. The wg.Add(1) should be called right before whatever go func it's waiting on, and then that func needs to notify when done.
If we make it a rule then we can take out that one line that was kinda useless as @itswisdomagain pointed out. Edit: I want to leave that line in after all. The server itself is also waiting for shutdown.

And then pass the context instead of the channel. I'm kinda torn here, as we could just pass the channel, but it doesn't hurt to pass the entire context, does it? Anyway, I like it like this. Also wires the autoreconnect to watch the shutdown context's done channel, and removes the need to call stop.

@JoeGruffins
Copy link
Copy Markdown
Member Author

JoeGruffins commented Sep 24, 2019

Not sure what this travis error is all about. Guess I'll try the open close trick later...

@chappjc
Copy link
Copy Markdown
Member

chappjc commented Sep 24, 2019

Restarting the build won't help in this case. Go 1.13 now validates all pseudo version strings. gitea specified too much of one dep's commit hash.

You'll have to put a replace statement:

replace github.com/go-macaron/cors v0.0.0-20190309005821-6fd6a9bfe14e9 => github.com/go-macaron/cors 6fd6a9bfe14e

@JoeGruffins
Copy link
Copy Markdown
Member Author

@chappjc Thank you!!

Copy link
Copy Markdown
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

This can go in when the conflict is resolved

A struct channel was used in order to signal shutdowns. This changes
that to a context.Context that we can spawn new contexts from. Abstracts
the task to a seperate file signal.go
@dajohi dajohi merged commit 9eb9e06 into decred:master Sep 25, 2019
@JoeGruffins JoeGruffins mentioned this pull request Sep 28, 2019
jyap808 pushed a commit to ubiq/dcrstakepool that referenced this pull request Dec 16, 2019
A struct channel was used in order to signal shutdowns. This changes
that to a context.Context that we can spawn new contexts from. Abstracts
the task to a seperate file signal.go
ljk662 pushed a commit to bisontrails/dcrstakepool that referenced this pull request Mar 4, 2020
A struct channel was used in order to signal shutdowns. This changes
that to a context.Context that we can spawn new contexts from. Abstracts
the task to a seperate file signal.go
ljk662 pushed a commit to bisontrails/dcrstakepool that referenced this pull request Mar 4, 2020
A struct channel was used in order to signal shutdowns. This changes
that to a context.Context that we can spawn new contexts from. Abstracts
the task to a seperate file signal.go
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.

5 participants