Skip to content

Commit c93e0aa

Browse files
felixgery
authored andcommitted
Deprecate string interface for fs.read()
This patch makes buffers the preferred output for fs.read() and fs.readSync(). The old string interface is still supported by converting buffers to strings dynamically. This allows to remove the C++ code for string handling which is also part of this patch.
1 parent e84395f commit c93e0aa

File tree

5 files changed

+128
-115
lines changed

5 files changed

+128
-115
lines changed

doc/api.markdown

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1410,20 +1410,24 @@ specifies how many _bytes_ were written.
14101410

14111411
Synchronous version of `fs.write()`. Returns the number of bytes written.
14121412

1413-
### fs.read(fd, length, position, encoding, callback)
1413+
### fs.read(fd, buffer, offset, length, position, callback)
14141414

14151415
Read data from the file specified by `fd`.
14161416

1417+
`buffer` is the buffer that the data will be written to.
1418+
1419+
`offset` is offset within the buffer where writing will start.
1420+
14171421
`length` is an integer specifying the number of bytes to read.
14181422

14191423
`position` is an integer specifying where to begin reading from in the file.
1424+
If `position` is `null`, data will be read from the current file position.
14201425

1421-
The callback is given three arguments, `(err, data, bytesRead)` where `data`
1422-
is a string--what was read--and `bytesRead` is the number of bytes read.
1426+
The callback is given the two arguments, `(err, bytesRead)`.
14231427

1424-
### fs.readSync(fd, length, position, encoding)
1428+
### fs.readSync(fd, buffer, offset, length, position)
14251429

1426-
Synchronous version of `fs.read`. Returns an array `[data, bytesRead]`.
1430+
Synchronous version of `fs.read`. Returns the number of `bytesRead`.
14271431

14281432
### fs.readFile(filename, [encoding,] callback)
14291433

lib/fs.js

Lines changed: 45 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,7 @@ fs.readFileSync = function (path, encoding) {
9292
var pos = null; // leave null to allow reads on unseekable devices
9393
var r;
9494

95-
while ((r = binding.read(fd, 4*1024, pos, encoding)) && r[0]) {
95+
while ((r = fs.readSync(fd, 4*1024, pos, encoding)) && r[0]) {
9696
content += r[0];
9797
pos += r[1]
9898
}
@@ -143,14 +143,52 @@ fs.openSync = function (path, flags, mode) {
143143
return binding.open(path, stringToFlags(flags), mode);
144144
};
145145

146-
fs.read = function (fd, length, position, encoding, callback) {
147-
encoding = encoding || "binary";
148-
binding.read(fd, length, position, encoding, callback || noop);
146+
fs.read = function (fd, buffer, offset, length, position, callback) {
147+
if (!(buffer instanceof Buffer)) {
148+
// legacy string interface (fd, length, position, encoding, callback)
149+
var cb = arguments[4],
150+
encoding = arguments[3];
151+
position = arguments[2];
152+
length = arguments[1];
153+
buffer = new Buffer(length);
154+
offset = 0;
155+
156+
callback = function(err, bytesRead) {
157+
if (!cb) return;
158+
159+
var str = (bytesRead > 0)
160+
? buffer.toString(encoding, 0, bytesRead)
161+
: '';
162+
163+
(cb)(err, str, bytesRead);
164+
};
165+
}
166+
167+
binding.read(fd, buffer, offset, length, position, callback || noop);
149168
};
150169

151-
fs.readSync = function (fd, length, position, encoding) {
152-
encoding = encoding || "binary";
153-
return binding.read(fd, length, position, encoding);
170+
fs.readSync = function (fd, buffer, offset, length, position) {
171+
var legacy = false;
172+
if (!(buffer instanceof Buffer)) {
173+
// legacy string interface (fd, length, position, encoding, callback)
174+
legacy = true;
175+
encoding = arguments[3];
176+
position = arguments[2];
177+
length = arguments[1];
178+
buffer = new Buffer(length);
179+
180+
offset = 0;
181+
}
182+
183+
var r = binding.read(fd, buffer, offset, length, position);
184+
if (!legacy) {
185+
return r;
186+
}
187+
188+
var str = (r > 0)
189+
? buffer.toString(encoding, 0, r)
190+
: '';
191+
return [str, r];
154192
};
155193

156194
fs.write = function (fd, buffer, offset, length, position, callback) {

src/node_file.cc

Lines changed: 26 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -106,18 +106,9 @@ static int After(eio_req *req) {
106106

107107
case EIO_READ:
108108
{
109-
if (req->int3) {
110-
// legacy interface
111-
Local<Object> obj = Local<Object>::New(*callback);
112-
Local<Value> enc_val = obj->GetHiddenValue(encoding_symbol);
113-
argv[1] = Encode(req->ptr2, req->result, ParseEncoding(enc_val));
114-
argv[2] = Integer::New(req->result);
115-
argc = 3;
116-
} else {
117-
// Buffer interface
118-
argv[1] = Integer::New(req->result);
119-
argc = 2;
120-
}
109+
// Buffer interface
110+
argv[1] = Integer::New(req->result);
111+
argc = 2;
121112
break;
122113
}
123114

@@ -584,16 +575,6 @@ static Handle<Value> Write(const Arguments& args) {
584575
* 3 length integer. length to read
585576
* 4 position file position - null for current position
586577
*
587-
* - OR -
588-
*
589-
* [string, bytesRead] = fs.read(fd, length, position, encoding)
590-
*
591-
* 0 fd integer. file descriptor
592-
* 1 length integer. length to read
593-
* 2 position if integer, position to read from in the file.
594-
* if null, read from the current position
595-
* 3 encoding
596-
*
597578
*/
598579
static Handle<Value> Read(const Arguments& args) {
599580
HandleScope scope;
@@ -605,105 +586,47 @@ static Handle<Value> Read(const Arguments& args) {
605586
int fd = args[0]->Int32Value();
606587

607588
Local<Value> cb;
608-
bool legacy;
609589

610590
size_t len;
611591
off_t pos;
612-
enum encoding encoding;
613592

614593
char * buf = NULL;
615594

616-
if (Buffer::HasInstance(args[1])) {
617-
legacy = false;
618-
// 0 fd integer. file descriptor
619-
// 1 buffer instance of Buffer
620-
// 2 offset integer. offset to start reading into inside buffer
621-
// 3 length integer. length to read
622-
// 4 position file position - null for current position
623-
Buffer * buffer = ObjectWrap::Unwrap<Buffer>(args[1]->ToObject());
624-
625-
size_t off = args[2]->Int32Value();
626-
if (off >= buffer->length()) {
627-
return ThrowException(Exception::Error(
628-
String::New("Offset is out of bounds")));
629-
}
630-
631-
len = args[3]->Int32Value();
632-
if (off + len > buffer->length()) {
633-
return ThrowException(Exception::Error(
634-
String::New("Length is extends beyond buffer")));
635-
}
636-
637-
pos = GET_OFFSET(args[4]);
638-
639-
buf = (char*)buffer->data() + off;
640-
641-
cb = args[5];
642-
643-
} else {
644-
legacy = true;
645-
// 0 fd integer. file descriptor
646-
// 1 length integer. length to read
647-
// 2 position if integer, position to read from in the file.
648-
// if null, read from the current position
649-
// 3 encoding
650-
len = args[1]->IntegerValue();
651-
pos = GET_OFFSET(args[2]);
652-
encoding = ParseEncoding(args[3]);
595+
if (!Buffer::HasInstance(args[1])) {
596+
return ThrowException(Exception::Error(
597+
String::New("Second argument needs to be a buffer")));
598+
}
653599

654-
buf = NULL; // libeio will allocate and free it.
600+
Buffer * buffer = ObjectWrap::Unwrap<Buffer>(args[1]->ToObject());
655601

656-
cb = args[4];
602+
size_t off = args[2]->Int32Value();
603+
if (off >= buffer->length()) {
604+
return ThrowException(Exception::Error(
605+
String::New("Offset is out of bounds")));
657606
}
658607

608+
len = args[3]->Int32Value();
609+
if (off + len > buffer->length()) {
610+
return ThrowException(Exception::Error(
611+
String::New("Length is extends beyond buffer")));
612+
}
659613

660-
if (cb->IsFunction()) {
661-
// WARNING: HACK AGAIN, PROCEED WITH CAUTION
662-
// Normally here I would do
663-
// ASYNC_CALL(read, args[4], fd, NULL, len, offset)
664-
// but I'm trying to support a legacy interface where it's acceptable to
665-
// return a string in the callback. As well as a new Buffer interface
666-
// which reads data into a user supplied buffer.
667-
668-
// Set the encoding on the callback
669-
if (legacy) {
670-
Local<Object> obj = cb->ToObject();
671-
obj->SetHiddenValue(encoding_symbol, args[3]);
672-
}
673-
674-
if (legacy) assert(buf == NULL);
614+
pos = GET_OFFSET(args[4]);
675615

676-
677-
eio_req *req = eio_read(fd, buf, len, pos,
678-
EIO_PRI_DEFAULT,
679-
After,
680-
cb_persist(cb));
681-
assert(req);
616+
buf = (char*)buffer->data() + off;
682617

683-
req->int3 = legacy ? 1 : 0;
684-
ev_ref(EV_DEFAULT_UC);
685-
return Undefined();
618+
cb = args[5];
686619

620+
if (cb->IsFunction()) {
621+
ASYNC_CALL(read, cb, fd, buf, len, pos);
687622
} else {
688623
// SYNC
689624
ssize_t ret;
690625

691-
if (legacy) {
692-
#define READ_BUF_LEN (16*1024)
693-
char buf2[READ_BUF_LEN];
694-
ret = pos < 0 ? read(fd, buf2, MIN(len, READ_BUF_LEN))
695-
: pread(fd, buf2, MIN(len, READ_BUF_LEN), pos);
696-
if (ret < 0) return ThrowException(ErrnoException(errno));
697-
Local<Array> a = Array::New(2);
698-
a->Set(Integer::New(0), Encode(buf2, ret, encoding));
699-
a->Set(Integer::New(1), Integer::New(ret));
700-
return scope.Close(a);
701-
} else {
702-
ret = pos < 0 ? read(fd, buf, len) : pread(fd, buf, len, pos);
703-
if (ret < 0) return ThrowException(ErrnoException(errno));
704-
Local<Integer> bytesRead = Integer::New(ret);
705-
return scope.Close(bytesRead);
706-
}
626+
ret = pos < 0 ? read(fd, buf, len) : pread(fd, buf, len, pos);
627+
if (ret < 0) return ThrowException(ErrnoException(errno));
628+
Local<Integer> bytesRead = Integer::New(ret);
629+
return scope.Close(bytesRead);
707630
}
708631
}
709632

test/simple/test-fs-read-buffer.js

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
require('../common');
2+
var path = require('path'),
3+
Buffer = require('buffer').Buffer,
4+
fs = require('fs'),
5+
filepath = path.join(fixturesDir, 'x.txt'),
6+
fd = fs.openSync(filepath, 'r'),
7+
expected = 'xyz\n',
8+
bufferAsync = new Buffer(expected.length),
9+
bufferSync = new Buffer(expected.length),
10+
readCalled = 0;
11+
12+
fs.read(fd, bufferAsync, 0, expected.length, 0, function(err, bytesRead) {
13+
readCalled++;
14+
15+
assert.equal(bytesRead, expected.length);
16+
assert.deepEqual(bufferAsync, new Buffer(expected));
17+
});
18+
19+
var r = fs.readSync(fd, bufferSync, 0, expected.length, 0);
20+
assert.deepEqual(bufferSync, new Buffer(expected));
21+
assert.equal(r, expected.length);
22+
23+
process.addListener('exit', function() {
24+
assert.equal(readCalled, 1);
25+
});

test/simple/test-fs-read.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
require('../common');
2+
var path = require('path'),
3+
fs = require('fs'),
4+
filepath = path.join(fixturesDir, 'x.txt'),
5+
fd = fs.openSync(filepath, 'r'),
6+
expected = 'xyz\n',
7+
readCalled = 0;
8+
9+
fs.read(fd, expected.length, 0, 'utf-8', function(err, str, bytesRead) {
10+
readCalled++;
11+
12+
assert.ok(!err);
13+
assert.equal(str, expected);
14+
assert.equal(bytesRead, expected.length);
15+
});
16+
17+
var r = fs.readSync(fd, expected.length, 0, 'utf-8');
18+
assert.equal(r[0], expected);
19+
assert.equal(r[1], expected.length);
20+
21+
process.addListener('exit', function() {
22+
assert.equal(readCalled, 1);
23+
});

0 commit comments

Comments
 (0)