Do not call workSocket() in TNonblockigServer without ensuring that there is data on the socket #1497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make the work socket conditional Revert back the changes Only call workSocket() when there is pending data to read Minor update to a comment Only call workSocket() when there is pending data to read Only call workSocket() when there is pending data to read Fix the CMake build Fix the CMake build Fix the CMake build Use the correct type for ioctlsocket call Make non-blocking peek optional Revert "Make non-blocking peek optional" This reverts commit 501c440. Review fixes Review fixes Review fixes
|
@jeking3 The |
|
I also encounter the same error, but it does not show up every time I run the tests on Travis CI. |
|
I'm not aware of those being common or known. I have a bit of work to do in order to stabilize the CI builds and get them back to green... |
jeking3
reviewed
Mar 6, 2018
| // data sitting in their internal buffers and from libevent's perspective, there is no further data available. In | ||
| // that case, not having this workSocket() call here would result in a hang as we will never get to work the socket, | ||
| // despite having more data. | ||
| if (tSocket_->hasPendingDataToRead()) |
There was a problem hiding this comment.
Should this be an "if" or a "while"?
There was a problem hiding this comment.
It should be an if. At this point, we know the frame size. The next step is to read the frame data itself. And for that, there is already logic to work the socket until all the data is read. For SSL specifically, there is no concern that the SSL buffers can keep pending data forever, because when the entire data frame arrives, there won't be any incomplete SSL bytes. In any case, the code should work generically for all socket types.
jeking3
reviewed
Mar 6, 2018
| // data sitting in their internal buffers and from libevent's perspective, there is no further data available. In | ||
| // that case, not having this workSocket() call here would result in a hang as we will never get to work the socket, | ||
| // despite having more data. | ||
| if (tSocket_->hasPendingDataToRead()) |
There was a problem hiding this comment.
I am concerned this can cause an infinite loop if data is always available, since it calls itself.
There was a problem hiding this comment.
It can't. Note that the line above is transition(). So the next time we call workSocket() it will take a different case statement, as we are in a different state.
Btw, I assume you notice that the original workSocket call I removed, which was causing the superfluous exception, was reached via workSocket -> transition -> workSocket.
|
Rebase on master and let's see what happens, thanks. |
…/non-blocking-server-calls-workSocket-too-early
|
@jeking3 I think all tests have passed. |
|
Wow, haven't seen a green build in a while. Of course, none of the cross tests exercise nonblocking code. :( Would be nice to add this as another matrix option. |
|
What's the Jira ticket ID for this work? |
|
You need to create a Jira ticket in the THRIFT project (https://issues.apache.org/jira/projects/THRIFT/issues) that describes the bug and resolution so that I can merge it. I was able to test these changes out by making some changes to the cpp TestServer for crosstest which I will check in today. I changed my local tests.json file to add "--server-type=nonblocking" to each cpp server and changed some code to make additional cases work: (I think you can ignore the header failures) |
|
Cool so that last test run was without the fix (I thought it had it, but it only had my fixes to TestServer.cpp), and adding your fix seems to have taken care of the header failures somehow: |
jeking3
requested changes
Mar 11, 2018
There was a problem hiding this comment.
You need to create a Jira ticket in the THRIFT project (https://issues.apache.org/jira/projects/THRIFT/issues) that describes the bug and resolution so that I can merge it.
|
@jeking3 There is already a Jira item: https://issues.apache.org/jira/browse/THRIFT-4465 Btw, it makes sense that the fix resolves the framed transport failures. Because the bug was about the transition from the frame size reading to the frame content reading. |
jeking3
approved these changes
Mar 12, 2018
jeking3
pushed a commit
to jeking3/thrift
that referenced
this pull request
Mar 12, 2018
Client: cpp This closes apache#1497
ozymaxx
pushed a commit
to ozymaxx/thrift
that referenced
this pull request
Mar 20, 2018
- testing double rendering in JS, Java, Python and C++ - a comment added (about setprecision) - BOOST_TEST -> BOOST_CHECK double rendering in erlang, fixed (some additional tests added) less than operator - correction in erlang test style changes, some fixes in erlang test some fixes in erlang test some fixes in erlang test - no need for including lists library more style changes - something overlooked expected 2 lines before main - fixed (not seen locally) noticed that test cases should be added manually in Python DoubleConstantTest thrift compilation directive is now given in generate.cmake render_double_to_string -> emit_double_as_string added assertion error messages to see the problems inside Java tests in more detail RDP access to AppVeyor build workers THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 duplicate key problem in appveyor settings ignore the test cases with (-+)1.7e+308 in Java tests ignore the test cases with (-+)1.7e+308 in Java tests - 2 pyflakes8 style changes disabling the test named 'TestRenderedDoubleConstants' on machines with the MSVC2010 configuration msvc.profile parameter existence check passing "msvc profile" on build stage "MSVC_PROFILE" as a cache variable wrong usage of cache variables let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether TestRenderedDoubleConstants is excluded or not msvc.profile access - fixed msvc.profile access - fixed THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 TestRenderedDoubleConstants (in Java/Python) has been disabled on MSVC2013 builders THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 reverting changes on disabling a test depending on the MSVC compiler double emitting operation depending on the compiler version THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 emit_double_as_string refactored unnecessary carat has been removed
ozymaxx
pushed a commit
to ozymaxx/thrift
that referenced
this pull request
Mar 20, 2018
- testing double rendering in JS, Java, Python and C++ - a comment added (about setprecision) - BOOST_TEST -> BOOST_CHECK double rendering in erlang, fixed (some additional tests added) less than operator - correction in erlang test style changes, some fixes in erlang test some fixes in erlang test some fixes in erlang test - no need for including lists library more style changes - something overlooked expected 2 lines before main - fixed (not seen locally) noticed that test cases should be added manually in Python DoubleConstantTest thrift compilation directive is now given in generate.cmake render_double_to_string -> emit_double_as_string added assertion error messages to see the problems inside Java tests in more detail RDP access to AppVeyor build workers THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 duplicate key problem in appveyor settings ignore the test cases with (-+)1.7e+308 in Java tests ignore the test cases with (-+)1.7e+308 in Java tests - 2 pyflakes8 style changes disabling the test named 'TestRenderedDoubleConstants' on machines with the MSVC2010 configuration msvc.profile parameter existence check passing "msvc profile" on build stage "MSVC_PROFILE" as a cache variable wrong usage of cache variables let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether the MSVC_PROFILE parameter is passed correctly let's see whether TestRenderedDoubleConstants is excluded or not msvc.profile access - fixed msvc.profile access - fixed THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 TestRenderedDoubleConstants (in Java/Python) has been disabled on MSVC2013 builders THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 reverting changes on disabling a test depending on the MSVC compiler double emitting operation depending on the compiler version THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 emit_double_as_string refactored unnecessary carat has been removed THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 THRIFT-4474: Use PSR-4 autoloader by default Client: php This closes apache#1479 THRIFT-4516: Fix "go vet" warnings for Go 1.10 Client: go THRIFT-4419: Fix bug where framed messages > 4K could not be read Client: rs This closes apache#1508 THRIFT-4523 TStreamTransportImpl.GetOutputStream broken Client: Delphi Patch: Jens Geyer THRIFT-4515: cross server test improvement: graceful test server shutdown This closes apache#1509 THRIFT-82: move to SBCL 1.4.5 (hopefully will address 1.4.4 sporadic build errors)
danielhtshih
pushed a commit
to danielhtshih/thrift
that referenced
this pull request
Mar 20, 2018
Client: cpp This closes apache#1497
danielhtshih
added a commit
to danielhtshih/thrift
that referenced
this pull request
Mar 20, 2018
THRIFT-4490 Allow a default service as fallback for multiplex processors connected by old clients Client: Delphi Patch: Jens Geyer THRIFT-4492 protected ExceptionType type member of TApplicationException cannot be accessed Client: C# Patch: Jens Geyer This closes apache#1493 THRIFT-4352: update artful to use haxe 3.4.4 which fixes a core in haxe THRIFT-82: Add Common Lisp support Client: cl There's framed and buffered socket transport, binary protocol, multiplex, simple server, cross-tests, self-tests, tutorial, CL library, CL code generator. Only SBCL is supported for now. This closes apache#1412 THRIFT-82: follow-up to common lisp to stabilize the linux build environment and update some docs THRIFT-82: fix cl test server to return the correct SecondService string and enable multi tests THRIFT-4503: fix dlang server logging on client disconnect so it does not report an error for normal operation THRIFT-4498: add phpcs back Client: php This closes apache#1498 THFIFT-4486: golang: support https from generated clients golang: don't generate -remote.go clients when there are no functions golang: support for http headers in -remote clients golang: serialize cli requests with TJsonProtocol since serialization is not supported with TSimpleJsonProtocol Client: golang This closes apache#1488 Add nodejs test cases for unix domain socket Indicate that nodejs now supports unix domain socket THRIFT-4436: port nodejs changes from THRIFT-3748 to js lib, test for serialization of nested list, run all tests when building js lib Client: js This closes apache#1457 THRIFT-4505: Fix python build on Vagrant Windows boxes Client: py This closes apache#1499 THRIFT-4506: fix use of assert for correctness in Java SASL negotiation Client: java Updated the languages matrix markdown documentation. Minor tweaks to the language matrix markdown documentation. THRIFT-4508: Fix node.js to be the desired version in docker ubuntu-artful image THRIFT-4354: fix php socket blocking behavior Patch: Robert Lu <robberphex@gmail.com> Client: php This closes apache#1384 THRIFT-4508: end trusty CI builds; handle nodejs 4.x LTS EOL; update docs THRIFT-4508: change windows CI builds to use current MSVC, one cygwin and one mingw build THRIFT-4480 - Handle seqid = 0 Client: js This closes apache#1487 move the newly supported socket to the correct programming lnguage THRIFT-4509: remove nodejs browser test Client: nodejs This closes apache#1501 fix wrong document @param in TBase.h Client: cocoa This closes apache#1504 THRIFT-4024: Skip() throws TProtocolException.INVALID_DATA on unknown data types Client: js This closes apache#1503 THRIFT-4495: Allow `undefined` for non-required Erlang records fields. Client: erl As of Erlang 19, the dialyzer static type-analysis tool no longer implicitly adds `undefined` to the allowed types for a field. This means that dialyzer will now complain about any non-required fields that are not explicitly initialed when creating a new record. This closes apache#1494 THRIFT-4497: Use `map()` field type for Erlang type for map struct fields. Client: erl The Thrift Erlang code generator previously generated fields with the `#{}` Erlang type for maps fields. In the Erlang type specification languages, however, `#{}` specifically means an empty map. This commit fixes the code to emit `map()` instead, which means the maps keys and values may be of any type. It would be possible to emit a field type such as `${keytype() => maptype()}`, but this commit does not do that. This closes apache#1495 THRIFT-4515: fix up nonblocking options and enable ssl for nonblocking in cpp server cross Client: cpp THRIFT-4465: Fix C++ TNonblockingServer and THRIFT_EAGAIN issues Client: cpp This closes apache#1497 THRIFT-4337: Able to set keyStore and trustStore as InputStream in the TSSLTransportFactory.TSSLTransportParameters Client: java This closes apache#1486 THRIFT-4515: fix windows build Client: cpp THRIFT-4448: Golang: do something with context.Context. Remove Go1.6 compatibility. Client: go This closes apache#1459 THRIFT-4517: disable ocaml in xenial because it is broken THRIFT-4337: fix javadoc build error related to changes THRIFT-4508: remove cygwin64 build in favor of MSVC2013 THRIFT-4509: * switch from grunt-external-daemon and grunt-shell to grunt-shell-spawn * update grunt to 1.0.2 * always use local copy of jquery and qunit * commit the package-lock files for npm keep versions stable Client: js This closes apache#1506 THRIFT-4509: add jslint and fix build script output THRIFT-4429: Make TThreadPoolServer.executorService_ available in inherited classes and refactor methods to be able customization Client: java This closes apache#1485 THRIFT-4513: Fix thrift compiler to generate constants in stable order. This closes apache#1505 remove uds type; use --domain-socket only implement createHttpUDSConnection API
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Only call workSocket() when there is pending data to read
Some of the original discussion can be found here: #1476