NVIDIA / aistore

AIStore: scalable storage for AI applications
https://aistore.nvidia.com
MIT License
1.23k stars 165 forks source link

Fix multiple response.WriteHeader calls bug #53

Closed liangdrew closed 6 years ago

liangdrew commented 6 years ago

io.CopyBuffer() implicitly calls WriteHeader(). http.Error() also calls WriteHeader().

VladimirMarkelov commented 6 years ago

Yes, I was thinking about adding a new function that only reports about error but does not write anything to headers. It won't be a big change - the current invalmsghdlr stays the same, place with CopyBuffer will call new "invalimsghdlr". As Python Zen says 'Explicit is better than implicit.' :)

usmanong commented 6 years ago

new function is fine, the old API should have been there in the first place. see how much code you have to duplicate

VladimirMarkelov commented 6 years ago

If it was not stack trace with 'Caller', the old function could have called new one and then wrote to headers

liangdrew commented 6 years ago

I can add a new function just to log the error, but this is not very clean either since the error must be returned by this new function so that http.Error() can use it as an argument in invalmsghdlr(). Also, there would be a bit of code duplication. I'll look at slightly bigger refactors to unwind a bit of this mess.

usmanong commented 6 years ago

what about a new function A() which has the code currently in invalmsghdlr() like: A(w, r, errstr, status, writeErr) { s := http.StatusText(status) + ": " + specific + ": " + r.Method + " " + r.URL.Path if _, file, line, ok := runtime.Caller(1); ok { if !strings.Contains(specific, ".go, #") { f := filepath.Base(file) s += fmt.Sprintf("(%s, #%d)", f, line) } } glog.Errorln(s) glog.Flush() http.Error(w, s, status) h.statsif.add("numerr", 1) }

the your new function calls like: A(w, r, errstr, http.badrequest, false)

and invalmsghdlr calls like: A(w, r, errstr, status, true)

liangdrew commented 6 years ago

I don't like having to add those two extra arguments to every call to this function, but you're right, that works.

usmanong commented 6 years ago

no one should call A() directly, they call your new function and the existing invalmsghdlr(), so nothing changes outside

liangdrew commented 6 years ago

Oh I see what you mean - having two wrappers for A(). I like that, but I also feel bad about two functions which only call other functions. If I can't think of anything better, I'll probably do this.