Skip to content
This repository was archived by the owner on Dec 20, 2024. It is now read-only.

Conversation

@jim3ma
Copy link
Member

@jim3ma jim3ma commented Jul 8, 2020

Ⅰ. Describe what this PR did

RawContent func returns a buffer io, when enable autoReset, the buffer io can be reused by AcquireBufferSize, so, if first piece is processed slow than an other, the faster piece will overwrite buffer.

func (p *Piece) RawContent(noWrapper bool) *bytes.Buffer {
	contents := p.Content.Bytes()
	length := len(contents)
	defer func() {
		if p.autoReset {
			p.ResetContent()
		}
	}()

	if noWrapper {
		return bytes.NewBuffer(contents[:])
	}
	if length >= 5 {
		return bytes.NewBuffer(contents[4 : length-1])
	}
	return nil
}

Ⅱ. Does this pull request fix one issue?

fix #1415

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

N/A

Ⅳ. Describe how to verify it

N/A

Ⅴ. Special notes for reviews

@pouchrobot
Copy link
Contributor

@jim3ma Thanks for your contribution. 🍻
Please sign off in each of your commits.

@jim3ma jim3ma force-pushed the fix-bufer-overwrite-issue branch from 3d7783a to 92903d3 Compare July 8, 2020 02:10
@jim3ma
Copy link
Member Author

jim3ma commented Jul 8, 2020

@lowzj @starnop PTAL

_, err := io.Copy(writer, piece.RawContent(noWrapper))
if piece.autoReset {
piece.ResetContent()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can not ResetContent here in writePieceToFile.
Because the same piece may be used in target writer queue after write finish in client writer queue.
https://github.com/dragonflyoss/Dragonfly/blob/master/dfget/core/downloader/p2p_downloader/client_writer.go#L223

Copy link
Member Author

Choose a reason for hiding this comment

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

when enable cross write, we will read piece content twice? but this is a buffer, it has index, when read twice, I think we need reset some index, #1411 is a good workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

when enable cross write, we will read piece content twice? but this is a buffer, it has index, when read twice, I think we need reset some index, #1411 is a good workaround.

#1411 still can not solve this problem. Underlying Buf data will be reused. Piece should be reset after all writer process.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have rewritten the fix code #1411 , could you help me review whether it can work?

Copy link
Member Author

Choose a reason for hiding this comment

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

great!

@lowzj
Copy link
Member

lowzj commented Jul 14, 2020

IMOP, the pull request #1411 is an appropriate solution. I think that we can work on it to make it better.

@jim3ma jim3ma closed this Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dfget download file success with incorrect md5

4 participants