Issue 270090043: Julien Pierre's patch for full duplex selfserv

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (left):

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#oldco...
cmd/selfserv/selfserv.c:1395: if (!post || *post != 'P')
What was this check for?

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c
File cmd/selfserv/selfserv.c (right):

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:228: "-F enable full-duplex mode on /streaming URI\n"
Why on /streaming alone?

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:284: PRInt32      oserr     = PR_GetOSError();
Not all errors are os errors. you should probably check first somehow

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1092: PRIntervalTime maxInterval    =
PR_INTERVAL_NO_TIMEOUT;
You never seem to reset maxInterval, so just use PR_INTERVAL_NO_TIMEOUT
directly.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1140: }
See comments on this code in https://codereview.appspot.com/271820043/

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1151: atomics */
This comment needs editorial work.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1154: perWriteThread* data = (perWriteThread*) a;
This does not need a cast.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1155: PRFileDesc *	ssl_sock	= data->ssl_sock;
Weird spacing here.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1169: send_string(ssl_sock, defaultHeader);
Check for success

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1177: if (0 == toSend) {
if (!toSend)

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1181: }
See below: this isn't thread safe.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1186: while (toSend) {
Because the claimed behavior is matching chunk size, you should generate an
error if the chunk is too big, rather than break it up like ths.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1196: break;
Print some kind of error here.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1298: 
Declare variables at the top of the function.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1370: if (fullDuplex) {
if (fullDuplex && post) would make the code below simpler.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1373: if (post) {
vertical whitespace after declaration

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1376: uri++;
Put this inside the next block

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1379: char* space = PORT_Strchr(uri, ' ');
Why are you using strchr above and PORT_Strchr here? I would just use stdlib,
but please be consistent.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1387: if (uri && (0 == strcmp(uri, "/streaming"))) {
!strcmp()

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1390: perWriteThread tdata;
Declare at top of block.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1395: PR_CreateThread(PR_USER_THREAD, do_writes,
(void*)&tdata,
You do not need to cast pointers to (void *)

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1404: if (rv>0) {
if (rv > 0)

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1405: buf[rv] = 0;
Do you do anything with buf? If not, why are you bothering to null terminate.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1407: PR_AtomicAdd(&tdata.counter, rv);
This does not produce the claimed semantics of writing the same size chunks
since it consolidates chunks if there is no intermediate write.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1412: tdata.done = PR_TRUE;
I do not believe that this is safe.

Consider what happens when no data has been read.  tdata.counter is 0.
This thread waits on PR_recv() and the other thread busy-waits on the
PR_AtomicSet(). Eventually, PR_recv() returns 0 and then tdata.done is written
with PR_DONE and read in the other thread with no memory barrier.

https://codereview.appspot.com/270090043/diff/1/cmd/selfserv/selfserv.c#newco...
cmd/selfserv/selfserv.c:1415: PR_ASSERT(PR_SUCCESS == rc);
Check this, don't just assert it

Expand All Messages | Collapse All Messages