fs: reduced duplicate code in fs.write()

[ronkorving]

No description provided.

[thefourtheye]

LGTM

[ronkorving]

Amusingly btw, the names for strWrapper and bufWrapper were reversed:

• bufWrapper was used for strings, and its internal comment referred to strings
• strWrapper was used for Buffer, and its internal comment referred to Buffer

Anyway, that doesn't matter anymore as they've been unified now.

[thefourtheye]

CI Run: https://ci.nodejs.org/job/node-test-pull-request/339/

[vkurchatkin]

I think this was actually done on purpose to avoid polymorphism

[thefourtheye]

@vkurchatkin Could you please elaborate?

[mscdex]

@thefourtheye FWIW this blog post explains mono/poly/mega-morphism (how it relates to v8/JavaScript) pretty well.

[ajafff]

I think this was actually done on purpose to avoid polymorphism

Correct me if I'm wrong:
That does only apply if you access any property of the object, doesn't it?
Here we are just holding a reference and pass it to the callback -> no property access -> no polimorphism

[mscdex]

After looking at FSWrapReq, it looks like the function arguments are always the same types for a write operation, so I think @meinklaus is right here.

[ronkorving]

Any blockers?

[brendanashworth]

LGTM if the CI is happy. For the record, I don't even know if keeping a reference to the string is necessary ("external" strings? this isn't handled elsewhere afaik).

[ronkorving]

I don't know either to be honest. @trevnorris may know more on this. I don't see that as a blocker for this PR, but if we can get clarity on that, of course I can update the PR if needed. I wouldn't want to introduce risk though.

[trevnorris]

IIRC you had a fix like this before. LGTM. I'm the one who did it this way. Was b/c I was overly strict about where I placed that assignment. Wasn't actually needed.

[ronkorving]

In the previous PR it was about buffers, this time it's about a string (well, the conversation is). Are you saying I can remove buffer from that wrapper altogether? Or only in the case of strings? Or am I misunderstanding? In any case, it does seem like we need the wrapper to ensure we return the number of bytes written (although I have a hard time believing that C++ land won't return a number as the 2nd argument, but I can look into that).

[trevnorris]

I'm on crack. Was referring to changing the location of req.oncomplete = wrapper;

As for maintaining a reference to the string, when the string is externalized a reference has to be kept. Since we used the external memory from the string object. So if the string is GC'd while the data is written it can blow up.

[ronkorving]

Got it, thanks. Sounds like we're all good then :)

[trevnorris]

Yup. LGTM

[ronkorving]

Looking forward to seeing it merged :) Anything I can do to speed that up?

[trevnorris]

Whoops. Sorry. Running CI to verify everything is fine: https://ci.nodejs.org/job/node-test-pull-request/627/

[trevnorris]

Bugger. NM. ^F failed me, didn't see the CI run that @thefourtheye posted above.

Landed in c339fa3. Thanks much!

[jasnell]

@trevnorris@thefourtheye@vkurchatkin ... should this also go into v4.x?

[trevnorris]

@jasnell code cleanup, no bug fix but also no side-effects. might be useful to have if landing future patches in the same code in the future.

[jasnell]

Landed in v4.x-staging in fb3b848