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 #1266: Shutdown not working if using StartServer #1278

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@im-kulikov
Copy link
Contributor

commented Feb 11, 2019

  • added atomic CompareAndSwap
  • update servers, if StartServer called
  • fix data race

@im-kulikov im-kulikov referenced this pull request Feb 11, 2019

Closed

Shutdown not working if using StartServer #1266

3 of 3 tasks complete

@im-kulikov im-kulikov changed the title Fix #1266: Shutdown not working if using StartServer [WIP] Fix #1266: Shutdown not working if using StartServer Feb 11, 2019

@codecov

This comment has been minimized.

Copy link

commented Feb 12, 2019

Codecov Report

Merging #1278 into master will increase coverage by 0.26%.
The diff coverage is 94.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1278      +/-   ##
==========================================
+ Coverage   84.27%   84.53%   +0.26%     
==========================================
  Files          27       27              
  Lines        1990     2005      +15     
==========================================
+ Hits         1677     1695      +18     
+ Misses        205      202       -3     
  Partials      108      108
Impacted Files Coverage Δ
echo.go 89.8% <94.23%> (+1.56%) ⬆️

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 e3717be...4a3a6c5. Read the comment docs.

@im-kulikov im-kulikov changed the title [WIP] Fix #1266: Shutdown not working if using StartServer Fix #1266: Shutdown not working if using StartServer Feb 12, 2019

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2019

Any thoughts @vishr @alexaandru?

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

@vishr

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

ping @vishr @alexaandru

Definitely today, I have been busy lately.

@alexaandru

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

I'll review as soon as I can @im-kulikov Would you mind squashing the commits together? Thanks! :)

Fix #1266: Shutdown not working if using StartServer
- added atomic CompareAndSwap
- update servers, if StartServer called

@im-kulikov im-kulikov force-pushed the im-kulikov:fix/issue-1266 branch from f5be789 to 1785bc9 Feb 26, 2019

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

😔

@vishr
Copy link
Member

left a comment

@im-kulikov Why have you dropped exposed properties from Echo struct?

TLSListener net.Listener
smux *sync.Mutex
serve *http.Server
lis net.Listener
AutoTLSManager autocert.Manager

This comment has been minimized.

Copy link
@vishr

vishr Mar 1, 2019

Member

@im-kulikov Have you taken away these exposed properties?

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Why have you dropped exposed properties from Echo struct?

Race condition, as I write in PR description

@vishr

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Why have you dropped exposed properties from Echo struct?

Race condition, as I write in PR description

What if people are using it? @alexaandru

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

This breaks backward compatibility. In this case, this PR is for version 5.x

@vishr vishr added the v5 label Mar 1, 2019

@im-kulikov

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@vishr I can prepare a solution for the v4 branch, if you consider it necessary

im-kulikov added some commits Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.