Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'Content-Length' value returned by TSServer is one off on Windows #3403

Open
micbou opened this issue Jun 6, 2015 · 2 comments
Open

'Content-Length' value returned by TSServer is one off on Windows #3403

micbou opened this issue Jun 6, 2015 · 2 comments
Labels
API Relates to the public API for TypeScript Bug A bug in TypeScript Help Wanted You can do this
Milestone

Comments

@micbou
Copy link
Contributor

micbou commented Jun 6, 2015

The 'Content-Length' value returned by TSServer does not take into account that newlines on Windows are two characters \r\n instead of \n. The issue was found in PR ycm-core/ycmd#156.

Here's a minimal example. Create a dummy test.ts file and a Python script with the following lines:

#!/usr/bin/env python

import json
import subprocess


class TypeScript():
    _tsserver_handle = subprocess.Popen(
      'tsserver.cmd',
      stdout = subprocess.PIPE,
      stdin = subprocess.PIPE
    )

    def SendRequest(self, command, arguments=None):
        request = {
            'seq': 0,
            'type': 'request',
            'command': command
        }
        if arguments:
            request['arguments'] = arguments

        self._tsserver_handle.stdin.write(json.dumps(request) + '\n')


    def ReadResponse(self):
        headers = {}
        while True:
            headerline = self._tsserver_handle.stdout.readline().strip()
            if not headerline:
                break
            key, value = headerline.split( ':', 1 )
            headers[ key.strip() ] = value.strip()

        if 'Content-Length' not in headers:
          raise RuntimeError( "Missing 'Content-Length' header" )
        contentlength = int( headers[ 'Content-Length' ] )
        output = self._tsserver_handle.stdout.read( contentlength )
        print( repr( output ) )


def Main():
    typescript = TypeScript()
    test_file = 'test.ts'

    command = 'open'
    arguments = { 'file': test_file }

    typescript.SendRequest(command, arguments)

    command = 'reload'
    arguments = { 'tmpfile': test_file, 'file': test_file }

    typescript.SendRequest(command, arguments)
    typescript.ReadResponse()

    command = 'reload'
    arguments = { 'tmpfile': test_file, 'file': test_file }

    typescript.SendRequest(command, arguments)
    typescript.ReadResponse()


if __name__ == '__main__':
    Main()

This script sends requests to TSServer and tries to read them. An exception is raised if no Content-Length key in header is found.

On Linux, the output is:

'{"seq":0,"type":"response","command":"reload","request_seq":0,"success":true}\n'
'{"seq":0,"type":"response","command":"reload","request_seq":0,"success":true}\n'

On Windows:

'{"seq":0,"type":"response","command":"reload","request_seq":0,"success":true}\r'
Traceback (most recent call last):
  File "./typescript.py", line 68, in <module>
    Main()
  File "./typescript.py", line 64, in Main
    typescript.ReadResponse()
  File "./typescript.py", line 36, in ReadResponse
    raise RuntimeError( "Missing 'Content-Length' header" )
RuntimeError: Missing 'Content-Length' header

If contentlength is incremented by one:

'{"seq":0,"type":"response","command":"reload","request_seq":0,"success":true}\r\n'
'{"seq":0,"type":"response","command":"reload","request_seq":0,"success":true}\r\n'

In all these cases, TSServer returns a length of 78 characters ({"seq":0,"type":"response","command":"reload","request_seq":0,"success":true} is 77 long). It should return a length of 79 characters on Windows.

@mhegazy mhegazy added Bug A bug in TypeScript API Relates to the public API for TypeScript labels Jun 6, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Jun 6, 2015

Content-Length is initialized with 1 all the time, instead it should be host.newLine.length.

 this.sendLineToClient('Content-Length: ' + (1 + Buffer.byteLength(json, 'utf8')) +
                '\r\n\r\n' + json);

@mhegazy mhegazy added the Help Wanted You can do this label Jun 6, 2015
@benliddicott
Copy link

Alternatively sendLineToClient should either \n or \r\n on both platforms. Since this is an HTTP-like protocol I suggest CRLF for consistency.

In any case I suggest it would be better to send byte-for-byte the same output on all platforms.

(Some background for those not familiar:

HTTP and the web generally favour CRLF (it's specified in the MIME standard and also the format of multiline textboxes in web forms).

OTOH Linux and XML generally favour LF alone.

  • It's specified for XML which doesn't even have an option to preserve CR, even though this means there is effectively no such thing as MIME type text/xml (CR is required in text/* and effectively forbidden in XML).
  • And in Linux it's effectively forbidden as well (e.g. bash and many other tools will choke on CR as it isn't recognised as harmless whitespace by system() so if it appears on the end of a script line it will be tacked on to the last argument, generally resulting in some sort of failure).

)

@mhegazy mhegazy added this to the Community milestone Aug 11, 2015
homu added a commit to ycm-core/ycmd that referenced this issue May 21, 2016
[READY] Improve newline TypeScript fix on Windows

This change was suggested by @puremourning in PR puremourning#1.

With this change, we have nothing to do if issue microsoft/TypeScript#3403 is fixed and we will still support TSServer versions without the fix.

Also, according to this comment in the code:
> The response message is a JSON object which comes back on one line. Since this might change in the future, we use the 'Content-Length' header.

we should use `read` instead of `readline`.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/503)
<!-- Reviewable:end -->
@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Relates to the public API for TypeScript Bug A bug in TypeScript Help Wanted You can do this
Projects
None yet
Development

No branches or pull requests

4 participants