chore: update to go-log@v2 #8765
Conversation
| // 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. | ||
| // FIXME: We may block on read and not catch the context | ||
| // cancellation. | ||
| b.ReadFrom(pipeReader) | ||
| b.WriteTo(wnf) | ||
| select { | ||
| case <-r.Context().Done(): | ||
| return | ||
| default: | ||
| } | ||
| } | ||
| }() |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Blocking: need help.
BigLep
requested review from
guseggert
and removed request for
aschmahmann and
lidel
|
2022-04-07: we need to see if this has been superseded by go-libp2p work. |
| // FIXME: We are not handling read errors. | ||
| // FIXME: We may block on read and not catch the context | ||
| // cancellation. | ||
| b.ReadFrom(pipeReader) |
There was a problem hiding this comment.
This is a potential unbounded memory growth.
If we implement things like that we should use Read, Write loop with a fixed size buffer.
| b := new(bytes.Buffer) | ||
| go func() { | ||
| for { | ||
| // FIXME: We are not handling read errors. |
There was a problem hiding this comment.
The error seems handling seems easy enough.
An error while reading should be sent to the client and terminate the connection.
And an error while writing probably means the client disconnected, so just terminate the connection.
| 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. |
There was a problem hiding this comment.
That code is running in circles doing useless things:
https://github.com/ipfs/go-log/blob/8625e3ec81bdeb96627de192e6fe21eab5896603/pipe.go#L50-L58
r, w := io.Pipe() p := &PipeReader{ r: r, closer: w, core: newCore(opt.format, zapcore.AddSync(w), opt.level), }
Zap wants to write to an io.Writer and we have an io.Writer but we are doing zap -(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.
zap -(Write)> asyncBuffer -(sendDataOverChannelWithSelectOverAContextToo)> copyLoop -(Write)> HTTP
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.
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.
| select { | ||
| case <-r.Context().Done(): | ||
| return | ||
| default: |
There was a problem hiding this comment.
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 io.Pipe which blocks if there is no data).
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).
|
@schomatis you should also rebase on master too, a few things have changed, but AFAIK not the main blocking point. |
|
@schomatis : are you going to make the corresponding change/improvement in go-lobv2? |
|
@BigLep I'm confused on exactly what those changes should be. That's why I requested an issue to be created with the scoped changes needed to land this PR (if those are indeed a blocker here; I didn't completely understood the feedback here). |
|
Ok, I'll open an issue on go-log |
|
2022-04-26 conversation:
|
Ensuring people are aware the RPC API/CMD may change Context: #8765 (comment)
|
@BigLep Note that That can land in the next release if desired. |
Ensuring people are aware the RPC API/CMD may change Context: #8765 (comment)
Ensuring people are aware the RPC API/CMD may change Context: #8765 (comment)
|
@Jorropo : is there an issue in https://github.com/ipfs/go-log to improve things there? |
Need review and help in the flagged items in
core/corehttp/logs.go.Closes #8753.
Closes #9245