#32131 closed defect (fixed)

`SetDeadline not implemented` errors in proxy-go output

Reported by: dcf Owned by:
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords:
Cc: arlolra, cohosh, phw, dcf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

At da8b98d09089e32d53573a1cabcb450aa290b4c8, running proxy-go and getting assigned a client (as in comment:13:ticket:29258) causes a lot of output of this nature:

2019/10/17 19:47:02 connected to relay
2019/10/17 19:47:02 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:02 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:02 calling SetDeadline in Write returned the following error: SetDeadline not implemented
2019/10/17 19:47:02 Write 751 bytes --> WebRTC
2019/10/17 19:47:02 OnMessage <--- 126 bytes
2019/10/17 19:47:02 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:02 calling SetDeadline in Write returned the following error: SetDeadline not implemented
2019/10/17 19:47:02 Write 51 bytes --> WebRTC
2019/10/17 19:47:02 OnMessage <--- 40 bytes
2019/10/17 19:47:02 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:03 calling SetDeadline in Write returned the following error: SetDeadline not implemented
2019/10/17 19:47:03 Write 1508 bytes --> WebRTC
2019/10/17 19:47:03 OnMessage <--- 1057 bytes
2019/10/17 19:47:03 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:03 calling SetDeadline in Write returned the following error: SetDeadline not implemented
2019/10/17 19:47:03 Write 543 bytes --> WebRTC
2019/10/17 19:47:03 OnMessage <--- 1057 bytes
2019/10/17 19:47:03 calling SetDeadline in Read returned the following error: SetDeadline not implemented
2019/10/17 19:47:03 calling SetDeadline in Write returned the following error: SetDeadline not implemented

I suppose the immediate cause is the changes from #31794.

Child Tickets

Attachments (3)

Change History (12)

comment:1 Changed 12 months ago by cohosh

I agree that we should not be printing out this many warnings.

There are three ways to fix this:

  • undo these log messages from #31794 to not print out the returned error
  • change the SetDeadline (non)implementation to not return an error
  • implement SetDeadline for webRTCConn

The last option is perhaps the "most correct" route but also isn't really necessary for us since the timeout of the websocket connection will cause the copyloop to end. I'm hesitant to add more code that isn't necessary to snowflake since there is also a potential for adding bugs.

I guess I'd prefer just not logging the returned error.

comment:2 Changed 12 months ago by cohosh

Status: newneeds_review

Here's a patch.

comment:3 in reply to:  2 Changed 11 months ago by dcf

Status: needs_reviewmerge_ready

Replying to cohosh:

Here's a patch.

Now there's no reason for timeoutConn to exist, because its t member is never used, and its methods all pass through to Conn. So looks good to commit, then how about a second cleanup commit to remove timeoutConn?

comment:4 Changed 11 months ago by cohosh

Status: merge_readyneeds_review

Hmm, actually I just realized the deadline still could serve a purpose for the websocket connection to the bridge. I haven't come across an actual timeout in this sense... the bridge usually closes the websocket connection directly when the OR connection closes or times out. Do you know whether this code would ever be needed?

Anyway, here's a patch that removes the timeoutConn struct

comment:5 Changed 11 months ago by arlolra

Anyway, here's a patch that removes the timeoutConn struct

Maybe CopyLoopTimeout is no longer an appropriate name now.

comment:6 in reply to:  4 ; Changed 11 months ago by dcf

Status: needs_reviewmerge_ready

Replying to cohosh:

Hmm, actually I just realized the deadline still could serve a purpose for the websocket connection to the bridge.

You're right. I missed that. CopyLoopTimeout handles two different types of Conn, one that supports a timeout and one that doesn't.

I haven't come across an actual timeout in this sense... the bridge usually closes the websocket connection directly when the OR connection closes or times out. Do you know whether this code would ever be needed?

I agree the timeout is probably unnecessary on the WebSocket connection. We don't usually do that timeout thing in our other copy loops. Like arlolra says, CopyLoopTimeout should then become just CopyLoop or copyLoop.

comment:7 in reply to:  6 Changed 11 months ago by cohosh

Replying to dcf:

Replying to cohosh:

Hmm, actually I just realized the deadline still could serve a purpose for the websocket connection to the bridge.

I haven't come across an actual timeout in this sense... the bridge usually closes the websocket connection directly when the OR connection closes or times out. Do you know whether this code would ever be needed?

I have never seen it used. I think we're okay to remove it.

I agree the timeout is probably unnecessary on the WebSocket connection. We don't usually do that timeout thing in our other copy loops. Like arlolra says, CopyLoopTimeout should then become just CopyLoop or copyLoop.

Thanks! Updated patch: 0001-Remove-now-unecessary-timeoutConn.patch (ignore the *.2.patch file)

comment:8 Changed 11 months ago by dcf

Good to merge.

comment:9 Changed 11 months ago by cohosh

Resolution: fixed
Status: merge_readyclosed

Merged in 11bd32f62e

Note: See TracTickets for help on using tickets.