Opened 11 years ago

Closed 10 years ago

#209 closed defect (fixed)

flappclient assumes input from stdin will not be unicode

Reported by: zancas Owned by: zancas
Priority: major Milestone: 0.6.5
Component: appserver Version: 0.6.4
Keywords: Cc:

Description (last modified by Brian Warner)

I've registered an application with a flappserver with the --accept-stdin flag. I then invoked the application with unicode type arguments. The flappserver behaves in a difficult to understand manner.

I've traced the data that I input (the unicode) as far as the appserver.services.Command.remote_feed_stdin method at which point it was still passing the data around. At some point subsequent to that the data was converted to ''.

This patch zooko wrote should make the client much pickier about the data types it can receive.

diff --git a/foolscap/appserver/client.py b/foolscap/appserver/client.py
index 44e6671..18ed0da 100644
--- a/foolscap/appserver/client.py
+++ b/foolscap/appserver/client.py
@@ -103,6 +103,8 @@ class RunCommand(Referenceable, Protocol):
         return self.d
 
     def dataReceived(self, data):
+        if not isinstance(data, str):
+            usage.error('stdin can accept only strings of bytes, not "%s"' % (type(data),))
         # this is from stdin. It shouldn't be called until after _started
         # sets up stdio and self.stdin_writer
         self.stdin_writer.callRemoteOnly("feed_stdin", data)
diff --git a/foolscap/appserver/services.py b/foolscap/appserver/services.py
index f75248e..749ef70 100644
--- a/foolscap/appserver/services.py
+++ b/foolscap/appserver/services.py
@@ -235,6 +235,8 @@ class Command(Referenceable):
         self.log_stdin = log_stdin
         self.closed = False
     def remote_feed_stdin(self, data):
+        if not isinstance(data, str):
+            usage.error('stdin can accept only strings of bytes, not "%s"' % (type(data),))
         if self.log_stdin:
             log.msg("stdin: %r" % data)
         self.process.write(data)

Change History (8)

comment:1 Changed 11 years ago by Zooko

Zancas: I don't think you clearly explained what the problem was. The behavior was that the flappserver silently discarded the data, so that the flapp running inside the flappserver, when reading from its stdin, would get an empty string returned (i.e., there were 0 bytes to read on its stdin). There was no error message or warning logged by the flappserver or sent to the flappclient.

So, the bug is: when you send a unicode to a flappserver as the argument to its "remote_feed_stdin" method, then instead of giving you an error, or coercing the unicode into a stream of bytes, it instead silently discards it.

comment:2 Changed 11 years ago by Zooko

Oh I see. Zancas wrote: "At some point subsequent to that the data was converted to . ". Meaning that the data was discarded. Too clever! Brian probably thought that was a typo or something. ☺

comment:3 Changed 11 years ago by Brian Warner

Component: unknownappserver
Summary: foolscap assumesinput from stdin will not be unicodeflappclient assumes input from stdin will not be unicode

comment:4 Changed 11 years ago by Brian Warner

Description: modified (diff)

comment:5 Changed 11 years ago by Brian Warner

Good idea, I'll apply that patch. In the future, could you include it as an attachment instead of inline? That'd make it easier for me to extract and apply.

Last edited 11 years ago by Brian Warner (previous) (diff)

comment:6 Changed 11 years ago by Brian Warner

Milestone: undecided0.6.5
Owner: set to zancas

Patch landed in [a4ee2fb]. Do we need anything else for this, or just the clear error message? Please close this ticket if you think the error message is enough.

comment:7 Changed 11 years ago by Zooko

Brian: do you think someone (maybe zancas) should write a unit test for this patch?

comment:8 Changed 10 years ago by Brian Warner

Resolution: fixed
Status: newclosed

Nope, I'm going to pretend that this falls into the realm of precondition checks, so the usage.error is effectively an AssertionError, and I'm ok with not having test coverage of those, especially when I'm in a mood to close out tickets :).

Note: See TracTickets for help on using tickets.