> On Feb. 27, 2014, 4:02 p.m., Matt Jordan wrote: > > /branches/11/res/res_http_websocket.c, lines 324-350 > > <https://reviewboard.asterisk.org/r/3248/diff/1/?file=54350#file54350line324> > > > > So, I always get nervous every time I see a 'sanity' check polling loop > > :-) > > > > I know Thava took a similar approach on the patch on ASTERISK-23099 > > without the sanity check: > > > > + if (ast_wait_for_input(session->fd, 100) > 0) { > > + while ((readlen = fread(&(buf[readnow]), 1, > > MAXIMUM_FRAME_SIZE, session->f)) < 1) { > > + int ferr = ferror(session->f); > > + int feoffile = feof(session->f); > > + ast_debug(3,"ast_websocket_read() fread error > > ferr=%d, feoffile=%d, returnval=%"PRIu32"\n", ferr,feoffile,readlen); > > + } > > + } > > > > I think your approach of checking for EAGAIN is better - was that to > > work through the case that you mention in the comments, where the fd says > > it is ready to be read, but in reality no data is available? > > Moises Silva wrote: > Yes the EAGAIN check is exactly because of that AFAIR
I guess here EAGAIN may be necessary because , here (in ws_safe_read), we try to read before checking the fd (ast_wait_for_input) in some instances within the ast_websocket_read. This patch is clean. But, I've a question: what's the purpose of calling fread() with partial lens . Why not use MAX_FRAME_SIZE, so that, the data in the fd or (ssl_buff) can be read in one shot. If fragmented, then call again with (MAX_FRAME_SIZE - readlen ). This way we may avoid too many unnecessary calls to fread() and also avoid, calling fread() before checking the fd (ast_wait_for_input).. - Thava ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/3248/#review10990 ----------------------------------------------------------- On March 2, 2014, 7:19 p.m., Moises Silva wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviewboard.asterisk.org/r/3248/ > ----------------------------------------------------------- > > (Updated March 2, 2014, 7:19 p.m.) > > > Review request for Asterisk Developers and rnewton. > > > Bugs: ASTERISK-21930 and ASTERISK-23099 > https://issues.asterisk.org/jira/browse/ASTERISK-21930 > https://issues.asterisk.org/jira/browse/ASTERISK-23099 > > > Repository: Asterisk > > > Description > ------- > > Several fixes for the WebSockets implementation in res/res_http_websocket.c > > * Flush the websocket session FILE* as fwrite() may not actually guarantee > sending > the data to the network. If we do not flush, it seems that buffering on the > SSL > socket for outbound messages causes issues > > * Refactored ast_websocket_read to take into account that SSL file descriptors > may be ready to read via fread() but poll() will not actually say so because > the data was already read from the network buffers and is now in the libc > buffers > > This should fix an issue that I have experienced and other users may have > reported [1][2][3], where > secure websockets wouldn't work, messages seem to not make it into Asterisk > > [1] http://lists.digium.com/pipermail/asterisk-users/2013-August/280175.html > [2] https://issues.asterisk.org/jira/browse/ASTERISK-21930 > [3] https://issues.asterisk.org/jira/browse/ASTERISK-23099 > > > Diffs > ----- > > /branches/11/res/res_http_websocket.c 409360 > > Diff: https://reviewboard.asterisk.org/r/3248/diff/ > > > Testing > ------- > > See ASTERISK-21930 for details on other users testing these changes. I did > both WS and WSS calls, confirmed audio works with chrome. This patch is for > Asterisk 11 as the issue is reported on Asterisk 11, but I tested a few > months ago and same issue existed on 12 and trunk. I created my own team > branches for those too (/team/moy/webrtc-11, /team/moy/webrtc-12, > /team/moy/webrtc-trunk) > > Confirmed working by Sean Bright on Jan 20, 2014 on Asterisk 11 (see > ASTERISK-21930 comment) > > > Thanks, > > Moises Silva > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
