Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify correct concurrent usage of segment.Writer.Sealed #22

Open
banks opened this issue Feb 9, 2023 · 0 comments
Open

Clarify correct concurrent usage of segment.Writer.Sealed #22

banks opened this issue Feb 9, 2023 · 0 comments

Comments

@banks
Copy link
Member

banks commented Feb 9, 2023

While reading this code the other day, I think I see a potentially racey access of some state in segment.Writer.

There is a test the uses it concurrently and that passes even with the race detector enabled. But it's only OK because it's making use of an implicit contract that Sealed is only called in the single writer thread and never concurrently with Append.

The int is the indexStart which is part of the writer state.

// writer state is accessed only on the (serial) write path so doesn't need
// synchronization.
writer struct {
// commitBuf stores the pending frames waiting to be flushed to the current
// tail block.
commitBuf []byte
// crc is the rolling crc32 Castagnoli sum of all data written since the
// last fsync.
crc uint32
// writeOffset is the absolute file offset up to which we've written data to
// the file. The contents of commitBuf will be written at this offset when
// it commits or we reach the end of the block, whichever happens first.
writeOffset uint32
// indexStart is set when the tail is sealed indicating the file offset at
// which the index array was written.
indexStart uint64
}

We note there that no synchronisation is needed because only the writer may access that state...

But then the Sealed method reads that property:

raft-wal/segment/writer.go

Lines 511 to 521 in 50b7503

// Sealed returns whether the segment is sealed or not. If it is it returns
// true and the file offset that it's index array starts at to be saved in
// meta data. WAL will call this after every append so it should be relatively
// cheap in the common case. This design allows the final Append to write out
// the index or any additional data needed at seal time in the same fsync.
func (w *Writer) Sealed() (bool, uint64, error) {
if w.writer.indexStart == 0 {
return false, 0, nil
}
return true, w.writer.indexStart, nil
}

We didn't state in the docs that Sealed must not be called concurrently with Append although that is how we expect it to be used.

It's minor as it won't affect WAL right now, however we should clean that up.

Possible options:

  1. Leave it and just document that Sealed may not be called concurrently with Append (and vice versa).
  2. Make all accesses atomic

Either way we should also:

  • Fix the comments on the writer state around where that state is used since they are a bit misleading right now.
@banks banks changed the title Possible race in segment.Writer.Sealed Clarify correct concurrent usage of segment.Writer.Sealed Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant