Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

Socket write encoding case sensitivity#1586

Closed
ajclarkedubit wants to merge 1 commit into
nodejs:masterfrom
ajclarkedubit:master
Closed

Socket write encoding case sensitivity#1586
ajclarkedubit wants to merge 1 commit into
nodejs:masterfrom
ajclarkedubit:master

Conversation

@ajclarkedubit

Copy link
Copy Markdown

Just spent ages debugging a problem with Node failing an assertion when sending messages along a socket - it turns out that I was specifying "UTF8" as the encoding value, which was not being recognised because it's uppercase.

So I've just added a line to lowercase the encoding value and maybe save someone else some pain!

@bnoordhuis

Copy link
Copy Markdown
Member

I don't know. Socket.write() is hot code, it should be as fat free as possible. I'd rather solve this by making the docs more explicit.

@koichik?

@koichik

koichik commented Aug 26, 2011

Copy link
Copy Markdown

@bnoordhuis

I'd rather solve this by making the docs more explicit.

Because most of functions such as a Buffer() accept encoding name by uppercase, I think that we should fix this problem.

But, it is not only 'utf8' but also 'ucs2', 'hex' and 'base64' that the number of characters and the number of bytes are different.
Therefore:

  • Buffer.write() should always set _charsWritten.
  • Socket._writeOut() should not care its encoding.

Or, as well as net_uv.js:

  • Socket._writeOut() should create new Buffer rather than reusing it.

@koichik

koichik commented Sep 3, 2011

Copy link
Copy Markdown

Buffer.write() always sets Buffer._charsWritten by #1633, Socket._writeOut() should not care its encoding.
@bnoordhuis - Can you review 55e6be1?

@koichik koichik closed this in fdbfc9c Sep 4, 2011
@koichik

koichik commented Sep 4, 2011

Copy link
Copy Markdown

@AjClarke - This has just fixed in the another way. Thanks for the report.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants