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

(7.0.3) reading STON off of stdin produces primitive failure #4309

Open
dalehenrich opened this issue Aug 13, 2019 · 13 comments

Comments

@dalehenrich
Copy link

commented Aug 13, 2019

When STONReader tries to read the following off of stdin in Pharo7.0.3:

ClassSet [
	StLObjOutSample {
		#name : #WS_,
		#count : 2
	},
	StLObjOutSample {
		#name : #StashAbstractTool,
		#count : 4
	}
]

the class ClassSet is not recognized, but in the process of trying to generate the class not found error, a primitive failure is signalled by #getPosition::

File class(ProtoObject)>>primitiveFailed:
File class(ProtoObject)>>primitiveFailed
File class>>getPosition:
StdioStream(AbstractBinaryFileStream)>>position
ZnCharacterReadStream(ZnEncodedStream)>>position
ZnCharacterReadStream(ZnEncodedReadStream)>>position
[ readStream position ] in STONReader>>error: in Block: [ readStream position ]
BlockClosure>>on:do:
STONReader>>error:
[ :notFound | 
acceptUnknownClasses
	ifTrue: [ object := STON mapClass new.
		self storeReference: object.
		self parseMapDo: [ :key :value | object at: key put: value ].
		object at: STON classNameKey put: notFound object ]
	ifFalse: [ self
			error: 'Cannot resolve class named ' , notFound object printString ] ] in STONReader>>parseObject in Block: [ :notFound | ...
BlockClosure>>cull:
Context>>evaluateSignal:
Context>>handleSignal:
NotFound(Exception)>>signal
NotFound class>>signalFor:
[ NotFound signalFor: name ] in [ Object allSubclasses
	detect: [ :class | class isMeta not and: [ class stonName = name ] ]
	ifNone: [ NotFound signalFor: name ] ] in STONReader>>lookupClass: in Block: [ NotFound signalFor: name ]
OrderedCollection(Collection)>>detect:ifFound:ifNone:
OrderedCollection(Collection)>>detect:ifNone:
[ Object allSubclasses
	detect: [ :class | class isMeta not and: [ class stonName = name ] ]
	ifNone: [ NotFound signalFor: name ] ] in STONReader>>lookupClass: in Block: [ Object allSubclasses...
[ self at: key put: aBlock value ] in IdentityDictionary(Dictionary)>>at:ifAbsentPut: in Block: [ self at: key put: aBlock value ]
IdentityDictionary(Dictionary)>>at:ifAbsent:
IdentityDictionary(Dictionary)>>at:ifAbsentPut:
STONReader>>lookupClass:
STONReader>>parseClass
[ reference := self newReference.
targetClass := self parseClass.
object := targetClass fromSton: self.
self setReference: reference to: object ] in STONReader>>parseObject in Block: [ reference := self newReference....
BlockClosure>>on:do:
STONReader>>parseObject
STONReader>>parseSimpleValue
STONReader>>parseValue
STONReader>>next
@svenvc

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

That is not an STON error, the STDIO stream do not seem to understand #position

@dalehenrich

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@svenc, I agree ... not sure what #position should answer for stdin, but primitive failure is not the best answer:)

@akgrant43

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

Hi Dale,

I'm not sure how far STON needs to move the stream, but you might have some success by wrapping stdin with a ZnPositionableReadStream. The class comments describe the use and limitations.

From memory: AbstractBinaryFileStream does handle position, but it will fail if the stream type doesn't support it, e.g. if you redirect stdin from a file you may find that everything works fine. But probably only on Linux and Mac (not Windows).

HTH,
Alistair

@dalehenrich

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@akgrant, this is stdin ... I cannot control the source of stdin ... stdin may be a file or may not be a file ... I am not necessarily looking for a workaround with this bug report ... my current workaround is to avoid STON errors on stdin:)

It seems that AbstractBinaryStream has implemented #position and #position: and someone chose to subclass StdioStream off of AbstractBinaryStream, so it seems that it is the responsibility of StdioStream to do the right thing ...

I will grant that giving me a character position on a non-file-based StdioStream may not be very useful at the end of the day, but a primitive failure is not really the right answer ... answering 0 would certainly be better than a primitive failure ...

@svenvc

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

STON does not need #position during normal processing. It is used during error reporting (as in 'error x at position y'). Given

error: aString
	| streamPosition |
	"Remain compatible with streams that don't understand #position"
	streamPosition := [ readStream position ]
		on: MessageNotUnderstood do: [ nil ].
	^ STONReaderError signal: aString streamPosition: streamPosition

returning nil or not implementing position would be OK

@akgrant43

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

this is stdin ... I cannot control the source of stdin ... stdin may be a file or may not be a file

I wasn't suggesting control, more just understanding what's there. :-)

STON does not need #position during normal processing. It is used during error reporting (as in 'error x at position y').

Sven, wouldn't ZnPositionableReadStream solve this?

returning nil or not implementing position would be OK

Not implementing position isn't really practical since it can be used sometimes. It is possible to test whether it will succeed by using File class>>fileDescriptorType:.

I'n not convinced that raising an exception is the wrong behaviour, although it would be nicer to have a more descriptive error. Either way, there's nothing stopping the STON exception handler Sven quotes above from being extended to deal with stdio streams.

@dalehenrich

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

I understand that I just might one of the first people to actually try using stdin for anything interesting:), so this is a learning experience for all of us ...

To be honest, I expect Stdio to do the "right thing", otherwise what's the point of having a class that purports to know what is going on?

Last time I had a problem using the Stdio streams I was told to use ZnCharacterReadStream, now I'm being told that ZnPositionableReadStream might work ... I don't know how anyone else will be able to figure these things out without going through the same process I'm going through ... and it makes sense (to me) that what I/we learn would be end up being encoded in the logic for producing the right combination of streams to use reading and writing stdin, stderr, and stdout in Stdio itself ...

The current implementation of StdioStream is certainly quite a hack ... don't you agree? #peek is sorta simulated but not well enough to actually stand on it's own except in the simplest of cases (thus the need for ZnCharacterReadStream) and now #position is sorta implemented but not well enough to work for all Stdio use cases ... If the StdinReadStream class did not implement position then Sven's suggestion would be appropriate because he wants STONReader to work on non-positionable streams ... but leaving a broken #position/#position: that Stdio should know about in advance is not a quality solution. When I see a primitiveError, I typically think that something is broken (as in this case) and masking the primitive error with a better error message is not necessarily a solution ...

If you have a class named Stdio and it's only job is to provide access to stderr, stdout, stdin, then it should be the place where all of these special cases are accounted for and provide a stream that will work with the underlying mechanism (whether it be positionable or not) ...

FileReference offers different flavors of binary/character; read/write streams and Stdio could do the same ... from reading the Stdio class comment there are actually 5 special cases for the underlying streaming model ... if there is enough knowledge to write the comment, there should be enough knowledge to actually provide a useful set of streams for all of the different cases ... and that is not quite happening with the current implementation (in my experience).

I want to reiterate, that my comments here are aimed at a real solution to the problems I am running into (If I hit them, I assume that I won't be the last one unless the underlying problems are fixed and they won't be fixed unless a bug is reported) ... I wouldn't be submitting bugs if I wasn't hoping to see Pharo improved ... I have a class whose job is to supply stdin/stderr/stdout (on a cross platform basis) and I can certainly write my code to do what I consider to be Stdio's job, but if I do that, Pharo isn't being improved ...

@ghost

This comment has been minimized.

Copy link

commented Aug 13, 2019

@dalehenrich

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

There should be a "mute the thread" link at the bottom of the emails you are receiving ... if you click on the link you should stop receiving emails for this issue - I assume:)

@svenvc

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Obviously, the Stdio streams have not been used enough. Thanks for testing & reporting.

My view is that positioning is not a fundamental concept of streaming, it is even in conflict with it. Positioning only makes sense when there is an underlying collection that is fully known. All kinds of special streams are not like that. Hence I always try to avoid using it.

ZdcAbstractSocketStream and subclasses (used for all HTTP/S in Pharo) do not implement positioning (how could they ?).

Not knowing the current Stdio implementation, my first reaction would be that positioning does not make sense there either.

ZnPositionableReadStream does indeed add positioning to another stream, but using a buffer (and thus a real cost). The class comment explains what it does. I would not use that in this case, it would make things more complex than they need to be.

@akgrant43

This comment has been minimized.

Copy link
Collaborator

commented Aug 14, 2019

My view is that positioning is not a fundamental concept of streaming, it is even in conflict with it. Positioning only makes sense when there is an underlying collection that is fully known. All kinds of special streams are not like that. Hence I always try to avoid using it.

While I agree that avoiding positioning has advantages, including making the code more generally useable, streams are the way we access files, and positioning within files is important in many cases.

Not knowing the current Stdio implementation, my first reaction would be that positioning does not make sense there either.

Stdio shares its primitives with file I/O on Unix (on Windows it shares the primitives, but uses readconsole() / writeconsole() for stdio). That's why redirecting stdin from a file allows positioning to be used. Obviously terminal and piped input doesn't support positioning.

@dalehenrich

This comment has been minimized.

Copy link
Author

commented Aug 14, 2019

I will restate my position one more time and then let you guys (pharo team) figure out what you want your users to have.

Stdio should in all cases return a usable stream for the underlying os implementation stdin/stder/stdout ... if a positionable stream can be used return ZnCharacterReadStream if a positionable stream cannot be used (create and) return ZnCharacterNonPosibitionableReadStream ... do the same sort of thing for binary flavors and write variants ... provide access to the raw byte stream (probably AbstractBinaryWriteStream) and I think you will have covered all of the bases ...

If this had been done in the first place, I would have gotten a MNU #position and I would have submitted a bug on the STON project that STONReader does not work with non-positionable streams:)

@svenvc

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Alistair, I believe you wrote the original code, or at least in its latest version.

What I would do is overwrite #position and #position: in StdioStream, calling super, catching the primitive failed error and throw an MNU. This should be acceptable since the pfe actually means that positioning is not supported (if I understood you correctly), hence that would agree with the method not being available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.