-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
chore: update to go-log@v2 #8765
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
package corehttp | ||
|
||
import ( | ||
"bytes" | ||
"io" | ||
"net" | ||
"net/http" | ||
|
||
core "github.com/ipfs/go-ipfs/core" | ||
lwriter "github.com/ipfs/go-log/writer" | ||
|
||
logging "github.com/ipfs/go-log/v2" | ||
) | ||
|
||
type writeErrNotifier struct { | ||
|
@@ -49,8 +51,32 @@ func LogOption() ServeOption { | |
mux.HandleFunc("/logs", func(w http.ResponseWriter, r *http.Request) { | ||
w.WriteHeader(200) | ||
wnf, errs := newWriteErrNotifier(w) | ||
lwriter.WriterGroup.AddWriter(wnf) | ||
log.Event(n.Context(), "log API client connected") //nolint deprecated | ||
|
||
// FIXME(BLOCKING): This is a brittle solution and needs careful review. | ||
// Ideally we should use an io.Pipe or similar, but in contrast | ||
// with go-log@v1 where the driver was an io.Writer, here the log | ||
// comes from an io.Reader, and we need to constantly read from it | ||
// and then write to the HTTP response. | ||
pipeReader := logging.NewPipeReader() | ||
b := new(bytes.Buffer) | ||
go func() { | ||
for { | ||
// FIXME: We are not handling read errors. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error seems handling seems easy enough. An error while reading should be sent to the client and terminate the connection. |
||
// FIXME: We may block on read and not catch the context | ||
// cancellation. | ||
b.ReadFrom(pipeReader) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a potential unbounded memory growth. If we implement things like that we should use |
||
b.WriteTo(wnf) | ||
select { | ||
case <-r.Context().Done(): | ||
return | ||
default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That code is a potential CPU spinner if you have a non blocking reader being returned. (which currently is not the case, currently this is an Just one more reason to make the correct thing (either just enroll it in the log framework, or make a custom asynchronous buffering thing and enroll that). |
||
} | ||
} | ||
}() | ||
Comment on lines
+55
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: need help. |
||
|
||
// FIXME(BLOCKING): Verify this call replacing the `Event` API | ||
// which has been removed in go-log v2. | ||
log.Debugf("log API client connected") | ||
Comment on lines
+77
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: need help. |
||
<-errs | ||
}) | ||
return mux, nil | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code is running in circles doing useless things:
https://github.com/ipfs/go-log/blob/8625e3ec81bdeb96627de192e6fe21eab5896603/pipe.go#L50-L58
Zap wants to write to an
io.Writer
and we have anio.Writer
but we are doingzap -(Write)> io.Pipe -(Read)> Read Write Repeat Loop -(Write)> HTTP Writer
.You should add support in go-log@v2 to add a writer into the core pool.
The only issue I see with that is that the syncronous nature of it could make scalling really slow (as it would write logs synchronously one by one).
And someone could do a slow loris attack and stuck the IPFS node.
The first approach we can say is it's the API and if you are so slow that you manage to slow your IPFS node it's your fault. With API access you could nuke the config anyway and we can't be expected to protect people from themselves.
If that really an issue we could implement a simple asynchronous buffered IO wrapper, that would cut off if there is too many buffered data.
Would be easy to do with channels (have write append to a channel and a goroutine read from that channel and forward the data to
Write
, you could also implement a custom ring buffer to coalles multiple reads). And since you are using channels in the copy loop, you can select on both the data input and the context done.If I was unclear just ask and I'll write that part, I already know what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an issue that should be raised and addressed in go-log, not here. I agree that things should be better on that side.