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

Node System host write - need to loop since writeSync might not write all bytes in one request #3682

Merged
merged 1 commit into from
Jul 13, 2015

Conversation

dbaeumer
Copy link
Member

fs.writeSync is speced in a way that it returns the byes written which is not handled correctly the the write function of the node system host.

We say issues with the tsserver when writing more than 64KB using fs.writeSync on the Mac running under electron (io.js)

@mihailik
Copy link
Contributor

io.js source indeed has a separate path for writing string (versus buffer), but it's not clear whether it flushes the whole string or just a part.

fs.js line 669:

fs.writeSync = function(fd, buffer, offset, length, position) {
  if (buffer instanceof Buffer) {
    if (position === undefined)
      position = null;
    return binding.writeBuffer(fd, buffer, offset, length, position);
  }
  if (typeof buffer !== 'string')
    buffer += '';
  if (offset === undefined)
    offset = null;
  return binding.writeString(fd, buffer, offset, length, position);  // <-- see here
};

Is there any way to reproduce a partial write for writeSync for string?

@mihailik
Copy link
Contributor

Also see fs.writeSync truncates long lines in io.js repo — seems to be a known bug?

},
while ((written = _fs.writeSync(1, buffer, offset, toWrite)) < toWrite) {
offset += written;
toWrite -= written;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe break if toWrite is 0?

@dbaeumer
Copy link
Member Author

dbaeumer commented Jul 1, 2015

@mihailik Regarding exposure: I am not sure I can follow the argument. The buffer API is only exposed to the node system host which is already very node specific. It depends on fs, path, ... For an emulated environment I would expect someone to implement the system host functions.

We only saw the issue on Mac under Electron which is based on io,js. It happend when the tsserver wrote a large response to the console (see #3758)

@dbaeumer
Copy link
Member Author

dbaeumer commented Jul 1, 2015

Sorry issue is #2758

@mihailik
Copy link
Contributor

mihailik commented Jul 1, 2015

@dbaeumer being node-specific is one thing, having large exposure to node API is another.

If you want to run tsc in a non-node environment, such as a browser, Rhino/Nashorn, FireFox OS — every new dependency on a platform is a headache. For me it's a very practical concern, I do run tsc emulating node api subset in browser.

Given that the root cause seems to be an io.js bug #1541, is there any way to limit the workaround to tsserver itself?

Tsserver is indeed much more dependent on V8/node, including ES5.
Tsc is ES3-compliant and very compatible with various environments.

@dbaeumer
Copy link
Member Author

dbaeumer commented Jul 6, 2015

@mihailik IMO the current node system is already buffer dependent. See the implementation of readFile. It reads the file as a buffer and checks the first two bytes for a BOM.

In general I think the tsc compiler should be programmed against the spec of writeSync even if in all implementation they currently write all content if it is a string. The implementation could change and still be spec compliant.

@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2015

👍

@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2015

@zhengbli can you give this a quick look.

@zhengbli
Copy link
Contributor

Confirmed it runs fine with Sublime Text plugin. 👍

@mhegazy
Copy link
Contributor

mhegazy commented Jul 13, 2015

thanks!

mhegazy added a commit that referenced this pull request Jul 13, 2015
Node System host write - need to loop since writeSync might not write all bytes in one request
@mhegazy mhegazy merged commit c549471 into master Jul 13, 2015
@mhegazy mhegazy deleted the nodeSystemHostWriteSync branch July 13, 2015 22:51
@zhengbli
Copy link
Contributor

Saw this problem again on Sublime with Node v4.2.1 on OSX. Even with the merged PR the problem still exists. The problem is the fs.writeSync would throw an exception EAGAIN: resource temporarily not available, watch, which terminates the partial write and crashes the server.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants