Stop attempting to close nil Listener #4753
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4753 +/- ##
=========================================
+ Coverage 63.45% 63.9% +0.44%
=========================================
Files 187 187
Lines 27472 27737 +265
=========================================
+ Hits 17433 17725 +292
+ Misses 8883 8856 -27
Partials 1156 1156
Continue to review full report at Codecov.
|
ebozduman
force-pushed
the
crashFSmode4715
branch
2 times, most recently
from
b5f36d9
to
dae8e02
Compare
pkg/http/server.go
Outdated
| if atomic.LoadInt32(&srv.upAndRunning) <= 0 { | ||
| return nil | ||
| } | ||
|
|
||
| // Close underneath HTTP listener. | ||
| srv.listenerMutex.Lock() | ||
| err := srv.listener.Close() |
There was a problem hiding this comment.
You could check srv.listener == nil than bringing additional field to the structure.
There was a problem hiding this comment.
Checking Listener for nil was my first approach, but then I discussed it with @harshavardhana and he suggested to implement the fix this way by introducing additional field in the structure.
As a continuation, the goal is to refactor the code more. So, the plan is to get rid of the mutex logic from Listener code/design. We can discuss it further on the phone if you'd like.
There was a problem hiding this comment.
I see now @ebozduman checking for listener as nil is fine if we are not resetting or reading the value elsewhere for this fix.
The refactor discussions requires lot more work we can defer that discussion for later date.
There was a problem hiding this comment.
Done.
@balamurugana , @harshavardhana , @donatello ,
Please review it again.
Thanks.
pkg/http/server.go
Outdated
| @@ -123,6 +123,10 @@ func (srv *Server) Shutdown() error { | |||
| return errors.New("http server already in shutdown") | |||
| } | |||
|
|
|||
| if srv.listener == nil { | |||
| return nil | |||
There was a problem hiding this comment.
-
A racy Start() and Stop() would set
srv.inShutdownincremented. It would look like the server is in shutdown. You could move this check aboveif atomic.AddUint32(...block. -
How about returning an error
server not initializedthannil?
ebozduman
force-pushed
the
crashFSmode4715
branch
from
e9258de
to
cf53583
Compare
|
commit message like |
ebozduman
force-pushed
the
crashFSmode4715
branch
from
cf53583
to
66dc3cf
Compare
balamurugana
changed the title
Description
If HTTP server is not up and running and if the Listener is attempted to be closed when the server could not even started up, then the offending panic/crash happens.
To prevent crash from happening, this fix introduces a new HTTP server variable
upAndRunningto keep track of the server's state. When we try to shut down the server, we just check if it was up before moving ahead with cleaning things up. This prevents the panic causing close attempt on the non-existing Listener.Motivation and Context
Crash in FS mode when Erasure coded mode is already running #4715
How Has This Been Tested?
Started Minio in distributed mode and tried to start a 2nd Minio with one of the disks/resources that has been already used by the other Minio instance.
Also tested the opposite case.
Types of changes
Checklist: