fix!: trace file seeking #1340
fix!: trace file seeking #1340
Conversation
evan-forbes
requested review from
ramin and
staheri14
and removed request for
a team
evan-forbes
added
WS: Big Blonks 🔭
| f.mut.Lock() | ||
| defer f.mut.Unlock() |
There was a problem hiding this comment.
Can we just rely on the mut to control access to the file? The reading and mut seem to be locked/set to true and unlocked/set to false simultaneously in all cases.
There was a problem hiding this comment.
good point, I'll take a look and see if we can simply this. The initial add for two different mutexes was so that we can ignore writes on line 43 w/o actually holding the mutex. Perhaps we can only use the reading atomic
There was a problem hiding this comment.
I tried a few things and unfortunately I think we'll have to keep both.
we don't want to only use the mutex, as that will block writes when reading, which could halt the node. If we only check if the mutex is locked then we risk ignoring writes even though we aren't reading. If we only use the reading atomic bool then we hit data races.
There was a problem hiding this comment.
will block writes when reading
I thought that is intended to be this way based on the godoc for bufferedFile
// bufferedFile is a file that is being written to and read from. It is thread
// safe, however, **when reading from the file, writes will be ignored**.Maybe that comment needs some revisions?
There was a problem hiding this comment.
will block writes when reading
I thought that is intended to be this way based on the godoc for bufferedFile
yeah we can definitely rephrase that. I think I'm some author bias unfortunately and need some help making it clearer, naively this part seems to indicate that writes are ignored.
when reading from the file, writes will be ignored.
perhaps a modification to the part on thread safety?
| return nil | ||
| } | ||
|
|
||
| func (f *bufferedFile) stopReading() error { |
There was a problem hiding this comment.
[suggestion] I'd rename it to something that indicates the seek is reset, and not paused e.g., resetCursor or resetSeek.
There was a problem hiding this comment.
this is a good point, one of the actions that is being done is that the cursor is being set to the end of the file (the last place we wrote to), however from the user's perspective the cursor of where we write isn't actually moving. What we are doing tho is setting atomic bool to false that indicates that the file is currently being read from
| func (f *bufferedFile) File() (*os.File, error) { | ||
| err := f.Flush() | ||
| // File returns the underlying file with the seek point reset. The caller should | ||
| // not close the file. The caller must call the returned function when they are |
There was a problem hiding this comment.
The caller must call the returned function when they are done reading from the file.
Can you provide more details on this? the purpose of the returned func is not clear (why should it be called when the reading is done, it does not look like that it closes the file)
There was a problem hiding this comment.
it should just be called to set the cursor to the end of the file and so that we stop ignoring writes.
| ) | ||
|
|
||
| // bufferedFile is a file that is being written to and read from. It is thread | ||
| // safe, however, when reading from the file, writes will be ignored. |
There was a problem hiding this comment.
This description suggests that writes and reads cannot be done simultaneously, hence my interpretation is that having one mutex to control access to the file is sufficient. There is no need for the reading bool.
Also, there is no possibility of concurrent reads based on the current implementation of startReading() (it always locks on mut hence disallowing concurrent reads). Hence, relying on mut should be fine.
Can you shed light on a case where two of reading and mut are needed? thanks
Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>
| if f.reading.Load() { | ||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
re-iterating an oddity for reviewers to hopefully help get this across the line. This seems silly to have an atomic and a mutex, however if we want to have the ability to ignore writes while also avoiding all races, then we need both the atomic and the mutex
|
recently added more reviewers to try and get this across the line. This is strictly a backport to main from v0.34.x-celestia, as this fix was already included there |
| type bufferedFile struct { | ||
| mut *sync.RWMutex | ||
| // reading protects the file from being written to while it is being read | ||
| // from. This is needed beyond in addition to the mutex so that writes can |
There was a problem hiding this comment.
| // from. This is needed beyond in addition to the mutex so that writes can | |
| // from. This is needed in addition to the mutex so that writes can |
Description
This PR backports the fix to the writing and reading of traced files. This enables them to get push correctly every time instead of occasionally get cut from the seek being set to the beginning of the file.
it also changes the api by unexporting a function that doesn't need to be exported or included in the interface.
these changes are included in the upcoming #1295