You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@thrift.apache.org by captncraig <gi...@git.apache.org> on 2014/08/08 20:20:20 UTC

[GitHub] thrift pull request: C# - reduce number of server exceptions

GitHub user captncraig opened a pull request:

    https://github.com/apache/thrift/pull/177

    C# - reduce number of server exceptions

    The current server implementation reads in a loop until the client closes, at which point an exception is thrown by the processor/transport and caught by the server.
    
    I solve this by creating a smarter peek functionality in TTransport which reads and caches a byte from the underlying transport and reinserts it in ReadAll.  

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/captncraig/thrift transport-peek

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/thrift/pull/177.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #177
    
----
commit 342f5370468fe2f32663687120612f2154a7a184
Author: Craig Peterson <cp...@ancestry.com>
Date:   2014-08-08T18:17:23Z

    Adding smarter peek functionality to prevent Transport Exception on every request.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request: C# - reduce number of server exceptions

Posted by asfgit <gi...@git.apache.org>.

Github user asfgit closed the pull request at:

    https://github.com/apache/thrift/pull/177


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request: C# - reduce number of server exceptions

Posted by captncraig <gi...@git.apache.org>.

Github user captncraig commented on the pull request:

    https://github.com/apache/thrift/pull/177#issuecomment-56225456
  
    Yeah, I agree that scenario sucks. The problem is, I am not sure that all of the transports have a way to really do peek. TcpSocket for example only has a Read operation. I would then have to do essentially the same code in each individual transport to achieve the same effect. As far as I could tell, all of the protocol code only calls ReadAll directly.
    
    Maybe it would be better to have to override peek in each individual transport, but I was trying to accomplish with as minor a change as possible.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request: C# - reduce number of server exceptions

Posted by Jens-G <gi...@git.apache.org>.

Github user Jens-G commented on the pull request:

    https://github.com/apache/thrift/pull/177#issuecomment-51640422
  
    tracked in THRIFT-2654


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] thrift pull request: C# - reduce number of server exceptions

Posted by Jens-G <gi...@git.apache.org>.

Github user Jens-G commented on the pull request:

    https://github.com/apache/thrift/pull/177#issuecomment-56222777
  
    I like the idea. But Peek() isn't actually a peek, it is a read operation. 
    Let's just take this (theoretical) scenario:
    
    * call `transport.Peek()` to effectively read a Byte
    * now call `transport.Read()` instead of `transport.ReadAll()` 
    * depending on the Transport used, you may lose the peeked Byte, maybe read it later out of band.
    
    I haven't checked the entire code if that is currently the case, but I don't like even the possibility of this. To me it would unexpected that Read and ReadAll differ that much. 
    
    While we're at it, why don't we implement a real peek operation? Again, I like the idea!
      
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---