zeroshade opened a new pull request, #853: URL: https://github.com/apache/arrow-go/pull/853
### Rationale for this change Closes #55. The IPC stream `Writer` and `FileWriter` are not safe for concurrent use. Both share mutable state across `Write`/`Close` with no synchronization: - the `started` / `headerStarted` flags (checked-then-set in `Write`/`Close` → schema/header can be written more than once), - the `lastWrittenDicts` map (written from `writeDictionaryPayloads` → Go `fatal error: concurrent map writes`), - the dictionary `mapper`, the shared `compressors` slice, and the underlying `PayloadWriter` (concurrent `WritePayload` → interleaved/corrupted stream), - `Close` setting `pw = nil` while a `Write` is in flight → nil-deref / use-after-close. As reported, calling `Write` from multiple goroutines on one writer can write the schema multiple times and crash the sink. Running the new tests under `-race` against the old code reports many data races. ### What changes are included in this PR? - Add a `sync.Mutex` to `Writer` and `FileWriter`, held for the duration of the exported mutating methods **`Write`** and **`Close`**. - The unexported `start` / `checkStarted` helpers are only ever called from `Write`/`Close`, so they intentionally stay **unlocked** to avoid self-deadlock. - Document the concurrency guarantee on `Write`/`Close`. Calls are serialized; records are written in the order in which callers acquire the lock. The guarantee is freedom from data races and stream corruption — not a particular record ordering — which matches how the standard library documents the mutex-protected `gob.Encoder` and `log.Logger`. No public signatures change; this is additive (a new unexported field + locking + godoc). ### Are these changes tested? Yes. Two new `-race` regression tests in `arrow/ipc/writer_test.go`: - `TestWriterConcurrentWrite` — 16 goroutines `Write` to one stream `Writer`, then the stream is read back and asserted to contain exactly one schema message + 16 record batches. - `TestFileWriterConcurrentWrite` — same for `FileWriter`, asserting `NumRecords() == 16` on read-back. Verified that both **fail with data-race warnings** on the pre-change code and **pass** with the lock. The full `arrow/ipc/...` suite passes under `-race` (no deadlock/regression), and `go vet` is clean. ### Are there any user-facing changes? No breaking changes. `Write`/`Close` keep their signatures; concurrent callers now get defined, race-free behavior instead of corruption/panics, and single-threaded callers are unaffected (an uncontended mutex). ### Design note Apache Arrow C++ (`IpcFormatWriter`) and Java (`ArrowWriter`) treat IPC writers as single-threaded, and most Go stdlib stream writers (`bufio`, `csv`, `json`, `gzip`) leave synchronization to the caller. I went with the mutex (rather than only documenting non-safety) because the reported failure mode is a hard `concurrent map writes` crash, the lock is free when uncontended, and arrow-go already documents concurrency guarantees elsewhere (e.g. `FileReader.RecordBatchAt`/`RecordAt`). Happy to switch to a docs-only "caller must synchronize" approach if maintainers prefer matching C++/Java exactly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
