Make stdcopy.StdWriter thread safe.

[calavera]

Stop using global variables as prefixes to inject the writer header.
That can cause issues when two writers set the length of the buffer in
the same header concurrently.

Fixes #19950 (~~maybe?~~totally 🤘)

Signed-off-by: David Calavera david.calavera@gmail.com

[cpuguy83]

Not fixed :(

[calavera]

@cpuguy83 are you using @dpiddy's script to test it? I don't get that error anymore after applying this patch, but I might be missing something.

[cpuguy83]

Yes I am. It took a couple of runs but I did get it... also getting a panic from writing to the buffer (slice out of bounds).

[calavera]

yeah, I saw the panic, but I think it's a completely different issue, caused by using a bounded buffer.

[calavera]

@cpuguy83 can you try with this other script? bytes.Buffer is not thread safe on its own: https://gist.github.com/calavera/7d52bbfd677f898f8145

Please, make sure you're importing the right package and not a precompiled one without this patch.

[danp]

@calavera your script has a bug, it is calling Write on the buffer in Read

[danp]

Using your branch I still get things like:

time="2016-02-26T14:58:54-04:00" level=debug msg="framesize: 4" 
out
time="2016-02-26T14:58:54-04:00" level=debug msg="framesize: 4" 
out
time="2016-02-26T14:58:54-04:00" level=debug msg="framesize: 4" 
err
time="2016-02-26T14:58:54-04:00" level=debug msg="framesize: 4" 
time="2016-02-26T14:58:54-04:00" level=debug msg="framesize: 1701999114" 
time="2016-02-26T14:58:54-04:00" level=debug msg="Extending buffer cap by 1701966346 (was 32777)" 
time="2016-02-26T14:58:56-04:00" level=debug msg="Corrupted frame: [111 117 116 10 1 0 0 0 0 0 0 4 111 117 116 ...]"

Running with GOMAXPROCS=1 seems to make it work.

[calavera]

Thanks @dpiddy it should be fixed now.

I wonder what's different in my environment, I don't get any error and I don't have GOMAXPROCS set :/

➜  stdtests  echo $GOMAXPROCS

➜  stdtests
``

[danp]

I had to add this back to your script to get the debug output:

logrus.StandardLogger().Level = logrus.DebugLevel

Can you try that?

[calavera]

okay, I finally got that corruption error.

[danp]

Cool. I think it would be good to eventually fill out the focused test script a bit to include a single net.Conn for stdout & stderr. Much like you needed to lock reads and writes on the buffer I think something similar will need to be done when using a single net.Conn.

[calavera]

I still think this change fixes the original input header issue, fwiw 😛

[danp]

Should this be [stdWriterPrefixLen]byte{...} instead of [8]byte{...}?

[danp]

Maybe same for 0 -> stdWriterFdIndex?

[calavera]

yeah, there are a few hardcoded values that we might be able to replace by constants.

[danp]

Ah, looking over it now I see that stdcopy's Write calls Write twice, once for the header and once for the frame. Part of the bigger issue? Two stdcopy writers could interleave their header and frame writes to the same underlying writer.

[calavera]

I'm trying to change that.

[calavera]

@dpiddy, @cpuguy83 I made some changes, do you mind take a look?

[danp]

I think that's done it! I was able to run this new test script which uses a tls.Conn for over a minute without weirdness.

[LK4D4]

LGTM

[calavera]

🙌 thanks for checking @dpiddy 🎉

[cpuguy83]

LGTM

[icecrime]

retest this

[icecrime]

retest that

[icecrime]

please retest this

[icecrime]

test this please

[icecrime]

Great process, ⭐⭐⭐⭐⭐, would test again.

[cpuguy83]

Maybe sudo? :)

[icecrime]

Aaaaaaaand it's failing. Halp @SvenDowideit!

[calavera]

test this please 🙏

[runcom]

test this please

[cpuguy83]

Test is a known failure on win2lin.
Merging.