Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

bugfix: fix dfget panic when crossWrite is true #1411

Merged
merged 1 commit into from
Jul 15, 2020

Conversation

zcc35357949
Copy link
Contributor

Signed-off-by: zhouchencheng [email protected]

Ⅰ. Describe what this PR did

when dfget's --output and --home directories are mounted under different disk device, it makes the hard link between them fails to create. At that time, crossWrite will be true.

And the same piece will be written to different files twice. Currently the autoreset field in piece is always true, so the piece buffer will reset after the first written, then the second written will trigger panic.

Ⅱ. Does this pull request fix one issue?

fixes #1410

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project kind/panic Any issue reporting containing a panic priority/P1 this is high priority that all maintainers should stop to handle this issue size/S labels Jul 5, 2020
}

// RawContent returns raw contents,
// If the piece has wrapper, and the piece content will remove the head and tail.
func (p *Piece) RawContent(noWrapper bool) *bytes.Buffer {
contents := p.Content.Bytes()
length := len(contents)
defer func() {
if p.autoReset {
p.ResetContent()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not invoke TryResetContent here to automatically release buffer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not invoke TryResetContent here to automatically release buffer?

we should release the buffer after the io.Copy() finishes, otherwise the same buffer may be reused for writing at the same time. #1415

Copy link
Member

@lowzj lowzj Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about to provide a method WriteTo in piece struct to write the raw content to writer and then release the buffer if necessary, just like :

n, e := piece.WriteTo(writer, noWrapper)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a good idea,I will try it.

@@ -220,6 +221,7 @@ func (cw *ClientWriter) write(piece *Piece) error {
}

if cw.acrossWrite {
atomic.AddInt32(&piece.writerNum, 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to abstract this line as a method of struct Piece, such as Piece.IncWriter.

Copy link
Member

@jim3ma jim3ma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
if length >= 5 {
return bytes.NewBuffer(contents[4 : length-1]).WriteTo(w)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here could reuse p.RawContent(noWrapper) to get buffer.

@lowzj
Copy link
Member

lowzj commented Jul 14, 2020

please rebase your code from master to solve the CI problem

Copy link
Member

@lowzj lowzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lowzj lowzj merged commit 8ce59d0 into dragonflyoss:master Jul 15, 2020
lowzj added a commit to lowzj/Dragonfly that referenced this pull request Jul 16, 2020
bugfix: fix dfget panic when crossWrite is true
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug This is bug report for project kind/panic Any issue reporting containing a panic priority/P1 this is high priority that all maintainers should stop to handle this issue size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dfget panic when use it to download file in p2p pattern.
4 participants