fix(pkg/ioutil):avoid panic in PageWriter.Write() when pageBytes is 0 #14022
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahrtr
reviewed
May 9, 2022
ahrtr
reviewed
May 13, 2022
Codecov Report
@@ Coverage Diff @@
## main #14022 +/- ##
==========================================
+ Coverage 75.00% 75.03% +0.02%
==========================================
Files 450 446 -4
Lines 37173 36723 -450
==========================================
- Hits 27882 27555 -327
+ Misses 7522 7432 -90
+ Partials 1769 1736 -33
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Please also rebase this PR. |
|
@secsys-go Please update this PR per the comment. |
|
Please squash the commits. |
ahrtr
reviewed
May 24, 2022
Comment on lines
+29
to
+30
| if err := recover(); err != nil { | ||
| return |
There was a problem hiding this comment.
Suggested change
| if err := recover(); err != nil { | |
| return | |
| if err := recover(); err != nil { | |
| return |
Suggested change
| if err := recover(); err != nil { | |
| return | |
| if err := recover(); err == nil { | |
| t.Fatal("NewPageWriter should panic when pageBytes is 0") |
|
Again, please squash the commits into just one commit. FYI. git-squash |
added 2 commits
June 13, 2022 12:51
fix(pkg/ioutil):avoid panic in PageWriter.Write() when pageBytes is 0
fix(pkg/ioutil): Trigger a panic when pageBytes is illegal in NewPateWriter
update(Test pkg/ioutil):Update TestPageWriterPageBytes
migrate e2e & integration role_test to common
tests: Migrate Txn tests to common functional test framework
provide a generic assert function
server: Director can be stopped
Goroutine for new directors would live past director scope. Tests
could occassionally fail if this goroutine had log events after
test execution should have ended.
server: Add director interrupt handler
Director's goroutine would not be properly stopped in a non-test
scenario. Handler stops it when process is interrupted.
server: Move director interrupt handler to method
server: Don't register director interrupt handler
remove v2 http proxy in 3.6
tests: Extract cluster test cases
tests: Refactor spawn json command
hide the revision field when it isn't populated
tests: Make common framework context aware
Documentation: Publish v3.5 data inconsistency postmortem
scripts: Avoid additional repo clone
This PR removes additional clone when building artifacts.
When releasing v3.5.4 this clone was main cause of issues and
confusion about what release script is doing.
release.sh script already clones repo in /tmp/ directory, so clonning
before build is not needed. As precautions for bug in script leaving
/tmp/ clone in bad state I moved "Verify the latest commit has the
version tag" and added "Verify the clean working tree" to be always run
before build.
scripts: Detect staged files before building release
fix a typo: print the correct error info
Governance: Use lazy consensus when needed to make decision
In lack of supermajority, we sometimes required to hold on to
important decisions for long time. In order to speed up, after
giving enough time for supermajority, use lazy consensus.
Encapsulation of applier logic: Move Txn related code out of applier.go.
The PR removes calls to applierV3base logic from server.go that is NOT part of 'application'.
The original idea was that read-only transaction and Range call shared logic with Apply,
so they can call appliers directly (but bypassing all 'corrupt', 'quota' and 'auth' wrappers).
This PR moves all the logic to a separate file (that later can become package on its own).
Encapsulating applier logic: UberApplier coordinates all appliers for server
This PR:
- moves wrapping of appliers (due to Alarms) out of server.go into uber_applier.go
- clearly devides the application logic into: chain of:
a) 'WrapApply' (generic logic across all the methods)
b) dispatcher (translation of Apply into specific method like 'Put')
c) chain of 'wrappers' of the specific methods (like Put).
- when we do recovery (restore from snapshot) we create new instance of appliers.
The purpose is to make sure we control all the depencies of the apply process, i.e.
we can supply e.g. special instance of 'backend' to the application logic.
Marge applierV3Internal into applierV3 interface
Rename EtcdServer.Id with EtcdServer.MemberId.
It was misleading and error prone vs. ClusterId.
Applier does not depend on EtcdServer any longer.
All the depencies are explicily passed to the UberApplier factory method.
Move server/etcdserver/txn.go to new package: server/etcdserver/txn
Move etcdserver/errors.go to sepatate package to avoid cyclic dependencies.
Move apply to its own package (no dependency on etcdserver).
Apply encapsulation: Cleanup metrics reporting.
Side effect: applySec(0.4s) used to be reported as 0s, now it's correctly 0.4s.
Simplify imports and improve comments.
Move CheckTxnAuth to txn.
Rename package alising "apply2" -> apply.
Rename etcdserver/etcderrors package to etcdserver/errors.
expose UberApplier as interface (not as implementation struct).
Rename the txn, so as not to be the same as the package name.
Fixing missing comment on the dispatch() function.
Rename WrapApply to Apply.
Remove unused code and apply code-quality suggestions.
use go install instead of go get
feat(pkg/ioutil): verify.Assert is introduced into NewPageWritter
add etcd tool binaries into .gitignore
|
This is the squashed one:dc559c17979a1823fab7c80478c74b4db28e0f1b |
ahrtr
requested changes
Aug 8, 2022
|
Since there is no response from the author for a long time, so I resolved it in #14452 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
pkg/ioutil: add check for pageBytes when creating PageWriter
NewPageWriter() without check could return a PageWriter whose pageBytes
is equal to 0. This is fatal when buffer to write is longer than defaultBufferBytes,
and it will crashed in PageWriter.Write() with:
"panic: runtime error: integer divide by zero".
This check will prevent creating PageWriter in NewPageWriter()when pageBytes is 0,
and return a nil at that time.